ast: wrong type for Type method; have Type() Type, want Type() baz.NodeType

[mewmew]

I get the compile time errors listed below when trying to compile the Go generated ast for the following grammar (baz.tm):

Textmapper did not report any error for the grammar, which is why I was surprised to see an error reported from the Go compiler.

Note: this is on rev cbc923c1d47fa589f0a6a1e9372ec229dab5ea6d of Textmapper, using the following command to process the grammar @java -jar ${TM_DIR}/tm-tool/libs/textmapper-0.9.21.jar baz.tm

$ textmapper
lalr: 0.018s, text: 0.088s, parser: 29 states, 0KB

$ go install ./...
# github.com/mewspring/foo/baz/ast
ast/ast.go:122:41: invalid type for composite literal: TopLevelEntity
ast/factory.go:16:21: cannot use &ArrayType literal (type *ArrayType) as type BazNode in return argument:
	*ArrayType does not implement BazNode (wrong type for Type method)
		have Type() Type
		want Type() baz.NodeType
ast/factory.go:28:19: cannot use &TypeDef literal (type *TypeDef) as type BazNode in return argument:
	*TypeDef does not implement BazNode (wrong type for Type method)
		have Type() Type
		want Type() baz.NodeType

[mewmew]

I took a deeper look, and the main reason seems to be that the Node interface has the defined method Type() ll.NodeType, so the Type method is already taken; thus cannot be used in e.g. func (n TypeDef) Type() Type

While understandable, this seems a bit unfortunate to loose access to good keywords to be used in the AST. May I suggest adding a prefix to all (or most) method of the Node interface? For instance TMType instead of Type?

Edit: I know one possible solution is to simply rename Type in the AST by using an assignment in the grammar. If possible however, I would like to keep the grammar with the current chosen names.

E.g.

TypeDef -> TypeDef
	: LocalIdent '=' 'type' (OpaqueType | Foobar=Type)
;

Instead of:

TypeDef -> TypeDef
	: LocalIdent '=' 'type' (OpaqueType | Type)
;

[inspirer]

Yes, I agree that reserving a bunch of nice identifiers (such as Type, Parent, Child, and Children) is unfortunate for the grammar author but this is the only way for now since I embed the node rather than have it as a field. It seems that embedding should be eliminated here. Let me experiment if we can make it work without.

[mewmew]

I was struck by this issue again today.

The workaround is in commit https://github.com/llir/grammar/commit/1b802f1297e7e7938df92025967d6fc679da5df2

In essence I have an OffsetField nonterminal with the Offset attribute in the LLVM IR grammar.

 OffsetField -> OffsetField
   : 'offset:' Offset=IntLit
;

As there is a collision between Node.Offset and the Offset attribute of OffsetField, Textmapper renames the method to _Offset, with the signature func (n OffsetField) _Offset() IntLit.

I understand that something along these lines have to be done to avoid collisions with the Node.Offset name. However, as a user of Textmapper, this was both surprising and introduced a subtle bug as I could reference Offset but not _Offset from the code handling IR translation. Therefore, an incorrect value would be used (the offset in the input text, rather than the Offset attribute of the OffsetField).

To make users aware of these subtle issues, I would suggest reporting warnings when attribute names are used which collide with any of the Node interfaces methods. Also, since the method is renamed to start with an underscore _ it is unexported, and can thus not be referenced by users?

I would like to consider reporting warnings as an intermediate step towards fixing this issue, where a proper solution would involve either renaming the Node methods to names unlikely to collide with names used in user grammars. For instance each method could be prefixed with TM, which is not pretty, I agree, but pragmatic in this case.

Yes, I agree that reserving a bunch of nice identifiers (such as Type, Parent, Child, and Children) is unfortunate for the grammar author but this is the only way for now since I embed the node rather than have it as a field. It seems that embedding should be eliminated here. Let me experiment if we can make it work without.

Another thing that would be very interesting is of course if it would be possible to solve this issue in another way, as you hinted on earlier by eliminating Node embedding. I would be very curious to see how this approach turns out as it seem to be the cleanest one!

Cheers, Robin