Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go: Support 1.23 (Transparent aliases) #17358

Merged
merged 12 commits into from
Sep 6, 2024
Merged

Conversation

mbg
Copy link
Member

@mbg mbg commented Sep 3, 2024

This PR is an alternative to the line of work in #17058 which tries to keep the changes required to tolerate Go 1.23 more minimal. The main difference is that in #17058 we explicitly extract Alias types, store them in the database, and then consequently have to deal with type equality issues that arise from this. In contrast, we do not explicitly extract the Alias types in this PR and instead just use the Go compiler to resolve them to their underlying types directly. This has the benefit that much fewer changes are needed to tolerate Go 1.23 extraction and that the behaviour is the same as before Go 1.23. The downside is that no information about aliases is available to query writers.

@mbg mbg self-assigned this Sep 3, 2024
@github-actions github-actions bot added the Go label Sep 3, 2024
@smowton
Copy link
Contributor

smowton commented Sep 3, 2024

I think you will have a problem with the multiple fields (objects, in CodeQL Entity) representing the same struct-type's field.

For example, if we have

struct { x int }

type IntAlias = int
struct { x IntAlias }

Then we will call LookupObjectID twice for the two distinct objects (in Go 1.22 there would only have been one), and thanks to the extractor's ScopedObjectID routine I believe we will assign it different field IDs (I think we'll go down the FreshID path since fields don't directly exist at package scope) and fail to relate accesses to one object with the other.

Potential difference between the two approaches here: because you're conflating the two struct types in extractType, fieldstructs will only get populated for one of the two field objects.

@mbg mbg force-pushed the mbg/go/1.23-transparent-aliases branch from 7fca405 to 5fd2e87 Compare September 3, 2024 15:13
go/actions/test/action.yml Outdated Show resolved Hide resolved
@smowton
Copy link
Contributor

smowton commented Sep 3, 2024

Confirmed the above is not in fact a problem: because more than one type maps onto the same TRAP label (e.g. struct { x int } and struct { x IntAlias } will both individually go through extractType, because the memoisation there is keyed on the type (which is different) not its label (which is the same, being calculated with transparent aliases). This results in duplicate extractions, but no database inconsistencies since the trap import phase cleans up the duplicates. We may find extraction is substantially slowed down though, in which case we should suppress unnecessary re-extraction, e.g. by checking after label calculation whether a type with that label has in fact already been extracted.

@mbg mbg force-pushed the mbg/go/1.23-transparent-aliases branch 2 times, most recently from 168afee to 034c823 Compare September 4, 2024 11:36
@mbg mbg changed the base branch from main to rc/3.15 September 4, 2024 11:36
@mbg mbg force-pushed the mbg/go/1.23-transparent-aliases branch from 034c823 to f06fa18 Compare September 4, 2024 12:48
Comment on lines +1510 to +1523
// Determines whether the given type is an alias.
func isAlias(tp types.Type) bool {
_, ok := tp.(*types.Alias)
return ok
}

// If the given type is a type alias, this function resolves it to its underlying type.
func resolveTypeAlias(tp types.Type) types.Type {
if isAlias(tp) {
return types.Unalias(tp) // tp.Underlying()
}
return tp
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is all redundant and we can just use types.Unalias. But this can be done in a follow-up PR.

Suggested change
// Determines whether the given type is an alias.
func isAlias(tp types.Type) bool {
_, ok := tp.(*types.Alias)
return ok
}
// If the given type is a type alias, this function resolves it to its underlying type.
func resolveTypeAlias(tp types.Type) types.Type {
if isAlias(tp) {
return types.Unalias(tp) // tp.Underlying()
}
return tp
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted. Yes, none of this is necessary anymore with the current version of this PR since Unalias implements the same logic. Agree that we can remove it later since it doesn't impact the behaviour.

owen-mc
owen-mc previously approved these changes Sep 4, 2024
@mbg mbg dismissed owen-mc’s stale review September 4, 2024 17:39

The merge-base changed after approval.

@mbg mbg marked this pull request as ready for review September 5, 2024 17:56
@mbg mbg requested review from a team as code owners September 5, 2024 17:56
@owen-mc
Copy link
Contributor

owen-mc commented Sep 5, 2024

Now that 1.23.1 has been released, fixing a crash bug which was affecting us, shall we update to that?

@mbg mbg force-pushed the mbg/go/1.23-transparent-aliases branch from 5ec862d to 772bc9b Compare September 5, 2024 20:12
@mbg
Copy link
Member Author

mbg commented Sep 6, 2024

The CI failure for "Go Resolve Build Environment Tests" is expected until the corresponding, internal PR has been merged. QA experiment results look good. Merging now.

@mbg mbg merged commit d1b311f into rc/3.15 Sep 6, 2024
19 of 20 checks passed
@mbg mbg deleted the mbg/go/1.23-transparent-aliases branch September 6, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants