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/types: inner CompositeLit T{{x}} has no type #69092

Closed
adonovan opened this issue Aug 27, 2024 · 7 comments
Closed

go/types: inner CompositeLit T{{x}} has no type #69092

adonovan opened this issue Aug 27, 2024 · 7 comments
Assignees
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@adonovan
Copy link
Member

When the type checker encounters an ill-typed composite literal, it does not descend into any inner composite literals (because it has no type to propagate downwards) so they have no recorded type.

Minimal repro in the types playground: var _ = T{{x}}. Observe that the outer has type Invalid, the inner no type at all.

This is the root cause of #68918.

@prattmic
Copy link
Member

cc @griesemer @findleyr

@prattmic prattmic added this to the Backlog milestone Aug 27, 2024
@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 27, 2024
@griesemer griesemer self-assigned this Aug 27, 2024
@griesemer griesemer modified the milestones: Backlog, Go1.24 Aug 27, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616616 mentions this issue: go/types, types: always record a type for inner composite literals

@griesemer
Copy link
Contributor

@adonovan FWIW, the above CL addresses this particular issue. Note though that while in T{{x}} both the outer composite literal T{{x}} and the inner {x} now have an (invalid) type recorded with them, the x still won't have a type. This is because in Checker.recordTypeAndValue we don't record anything for operands that are invalid. We could change that but that may possibly have repercussions for tools working with incorrect programs (they may see invalid types where before there were no types).

More generally, it may be very difficult to impossible to provide even invalid types for expressions that cannot be properly type checked by the type checker. It seems that this may be better handled on the tools side, because the work only needs to be done if one expects to deal with incorrect programs in the first place.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/617475 mentions this issue: gopls/internal/test/marker: update regression test issue68918.txt

gopherbot pushed a commit to golang/tools that referenced this issue Oct 2, 2024
The fix for golang/go#69092 produces an extra error message in Go 1.24.
Ignore it.

Updates golang/go#68918.
Updates golang/go#69092.

Change-Id: I41ecff6fe916a04ed943e29ad10184d340ef84d5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/617475
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@adonovan
Copy link
Member Author

adonovan commented Oct 2, 2024

More generally, it may be very difficult to impossible to provide even invalid types for expressions that cannot be properly type checked by the type checker. It seems that this may be better handled on the tools side, because the work only needs to be done if one expects to deal with incorrect programs in the first place.

I was going to disagree, as I had believed that it was (modulo bugs) an invariant that every Expr had a type, but I just audited x/tools and found that nearly 100% of places that look at Info.Types handle a missing entry, or are dominated by a check that type-checking was error-free. This is too high to be a coincidence. Furthermore, I notice in the types playground that it is easy to make an ill-formed expression that has no type. So, I think we have generally been conservative in our tools, and that I made a mistake in filing this issue. The "workaround" in gopls was in fact just the correct defensive code.

Sorry for taking up your time. I do think it would be valuable to record the non-invariant at Info.Types.

@adonovan
Copy link
Member Author

adonovan commented Oct 2, 2024

I do think it would be valuable to record the non-invariant at Info.Types.

Huh, we already do:

type Info struct {
	// Types maps expressions to their types, and for constant
	// expressions, also their values. Invalid expressions are
	// omitted. [...]
	Types map[ast.Expr]TypeAndValue

How embarrassing.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/617416 mentions this issue: x/tools: be defensive after types.Info.Types[expr] lookups

gopherbot pushed a commit to golang/tools that referenced this issue Oct 14, 2024
This CL is the result of a quick audit
of x/tools for places that look in the Types map and do
not handle missing entries gracefully (unless dominated
by a check for welltypedness, such as RunDespiteErrors:false
in the analysis framework). In each case it either adds
a defensive check or documents the assumption.

See golang/go#69092 (comment)

Updates golang/go#69092

Change-Id: I3573512fd47ee4dca2e0b4bce2803b92424d7037
Reviewed-on: https://go-review.googlesource.com/c/tools/+/617416
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants