go/types: invalid identifiers are no longer type checked #71777
Comments
gabyhelp
added
the
BugReport
This mangles CGo identifier names to something like "_Cgo_foo" instead of using literal identifiers like "C.foo". This works around golang/go#71777. I don't like this solution, but I hope we'll find a better solution in the future. In that case we can revert this commit.
This mangles CGo identifier names to something like "_Cgo_foo" instead of using literal identifiers like "C.foo". This works around golang/go#71777. I don't like this solution, but I hope we'll find a better solution in the future. In that case we can revert this commit.
This mangles CGo identifier names to something like "_Cgo_foo" instead of using literal identifiers like "C.foo". This works around golang/go#71777. I don't like this solution, but I hope we'll find a better solution in the future. In that case we can revert this commit.
|
CC @griesemer |
|
Setting milestone to 1.25 since this affects TinyGo. If we decide to fix this we should backport to fix to 1.24. |
This mangles CGo identifier names to something like "_Cgo_foo" instead of using literal identifiers like "C.foo". This works around golang/go#71777. I don't like this solution, but I hope we'll find a better solution in the future. In that case we can revert this commit.
|
I have a workaround for now, using name mangling. |
mknyszek
added
the
NeedsInvestigation
This mangles CGo identifier names to something like "_Cgo_foo" instead of using literal identifiers like "C.foo". This works around golang/go#71777. I don't like this solution, but I hope we'll find a better solution in the future. In that case we can revert this commit.
This mangles CGo identifier names to something like "_Cgo_foo" instead of using literal identifiers like "C.foo". This works around golang/go#71777. I don't like this solution, but I hope we'll find a better solution in the future. In that case we can revert this commit.
This mangles CGo identifier names to something like "_Cgo_foo" instead of using literal identifiers like "C.foo". This works around golang/go#71777. I don't like this solution, but I hope we'll find a better solution in the future. In that case we can revert this commit.
|
Hi @aykevl ; I looked at this some more and I don't see an obvious "right" solution on the parser/type-checker side: the general problem is that we don't want to repeat errors reported by the parser side of things (incl. scanner, etc) in the type checker; i.e., the type checker "knows" (within reason) what the parser has checked already. Your code explicitly manipulates the AST and makes it incorrect. Currently (with the fix for #68183) this knowledge is implicit, in the code: the type checker knows to not complain about invalid identifiers. A more general approach might be to have a bit (a bool) in the Ident signaling to the type checker that an error was already reported by the parser (and in general this might apply to any node). Perhaps no extra bit is needed, but a singleton "invalid identifier" can be used to communicate this situation. We already have a BadExpr node, but we cannot easily use that for a bad identifier w/o changing significant parts of the go/ast API. Alternatively, the parser could ignore the error and let the type checker do all the work: that would solve your problem but it would also make the parser lenient in a way that seems wrong - after all its job is to check the grammar. A 4th option might be to have a types.Config flag to enable this (and possibly other) "strict" checking in the type checker, say for situations where there is no parser, and the AST is generated somehow (or updated). All of these are doable but all of these seem not commensurate with the problem at hand or might require API changes that we'd rather not do. Since name mangling seems to work for you, I am inclined to do nothing here. |
Go version
go version go1.24.0 linux/arm64
Output of
go envin your module/workspace:What did you do?
I ran the code here: https://go.dev/play/p/SzYsA_F4wwR
What did you see happen?
In Go 1.23, it reports the following error:
In Go 1.24, it reports only the following error:
What did you expect to see?
I expected the
C.FOOundefined error to still be reported.This is a regression from 5bd442a (see #68183). We relied on the previous behavior for CGo support in TinyGo: we replaced all C.foo strings with a literal
"C.foo"identifier and defined functions and constants and such with a similar name. This worked very well and led to nice error messages (such as the "undefined: C.FOO" message above, instead of having to use name mangling).I'm not sure how to best fix this. I see that 5bd442a fixes a real bug, but in our case it's a regression. We can use name mangling to work around this, making the CGo error messages less nice. We can do some search-and-replace on the error messages to work around the name mangling, but I'm afraid that's going to be fragile. Name mangling is probably the easiest short-term fix (since we need Go 1.24 support) but if there's any way this can be fixed upstream that would be great.
The text was updated successfully, but these errors were encountered: