Skip to content

[Go target] Fix for #3926: Add accessors for tree navigation to interfaces in generated parser #3927

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

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

j3r3miah
Copy link
Contributor

@j3r3miah j3r3miah commented Oct 11, 2022

What does this PR fix?

Discussion

This is an attempt to remove the need to always cast these interfaces to concrete types. I considered two options:

  1. Update accessors to return concrete types instead of interfaces
  2. Add the missing method signatures to the existing interfaces

This PR implements option (2), as (1) would be a breaking change for existing applications. I believe (2) is backwards compatible.

The strategy here is to add a flag, signature, to each of the ContextGetterDecl subclasses so that each of their corresponding template definitions can conditionally generate a method signature (for the interface) versus the complete method declaration.

The getSignatureDecl() functions on each of the ContextGetterDecl subclasses are pretty tedious... open to suggestions for a more elegant option...

Grammar

abb

module  : moduleData EOF   ;
moduleData : MODULE moduleName NEWLINE  dataList  NEWLINE* ENDMODULE ;

Before this change

Generated Go

func (p *abbParserParser) Module() (localctx IModuleContext) { ... }
func (s *ModuleContext) ModuleData() IModuleDataContext { ... }

type IModuleDataContext interface {
	antlr.ParserRuleContext

	// GetParser returns the parser.
	GetParser() antlr.Parser

	// IsModuleDataContext differentiates from other interfaces.
	IsModuleDataContext()
}

type ModuleDataContext struct {
	*antlr.BaseParserRuleContext
	parser antlr.Parser
}

Navigation in Go

module.(*parser.ModuleContext).ModuleData().(*parser.ModuleDataContext).ModuleName()

After this change

Generated Go

func (p *abbParserParser) Module() (localctx IModuleContext) { ... }
func (s *ModuleContext) ModuleData() IModuleDataContext { ... }

type IModuleDataContext interface {
    antlr.ParserRuleContext

    // GetParser returns the parser.
    GetParser() antlr.Parser

    // Getter signatures
    MODULE() antlr.TerminalNode
    ModuleName() IModuleNameContext
    AllNEWLINE() []antlr.TerminalNode
    NEWLINE(i int) antlr.TerminalNode
    DataList() IDataListContext
    ENDMODULE() antlr.TerminalNode

    // IsModuleDataContext differentiates from other interfaces.
    IsModuleDataContext()
}

type ModuleDataContext struct {
	*antlr.BaseParserRuleContext
	parser antlr.Parser
}

Navigation in Go

module.ModuleData().ModuleName()

Why is this PR important?

This change increases the usability of the Go parsers. Without this change, code littered with type assertions is difficult to read/maintain.

@j3r3miah
Copy link
Contributor Author

Hi @jimidle, I'd appreciate your review here

@jimidle
Copy link
Collaborator

jimidle commented Oct 12, 2022

I will look at this today. I notice one check does not pass though. We will need that to pass. On the face of it, this looks like the correct thing to generate

@j3r3miah
Copy link
Contributor Author

j3r3miah commented Oct 12, 2022

Thanks, @jimidle. I saw that failure as well - it looks like infra flake to me. It passed on the next commit.

Signed-off-by: Jeremiah Boyle <jeremiah.boyle@gmail.com>
Signed-off-by: Jeremiah Boyle <jeremiah.boyle@gmail.com>
@j3r3miah j3r3miah force-pushed the no_go_type_assertions branch from edcead0 to 9b2137f Compare October 12, 2022 02:19
@j3r3miah
Copy link
Contributor Author

Hey @jimidle, I'd appreciate that review when you have a chance. Thank you!

@kaby76
Copy link
Contributor

kaby76 commented Oct 28, 2022

Looks wonderful. The generated code is exactly what we need, and mirrors what the other targets do. @parrt

@parrt
Copy link
Member

parrt commented Oct 29, 2022

I can't tell if it's backward compatible...i don't wanna break code in google. ;)

I'll wait for @jimidle to sign off on this but looks better.

@j3r3miah
Copy link
Contributor Author

The only breaking scenario I can imagine is if user code is relying on implementing the parser-generated interfaces for some reason, which seems unlikely - but I'll defer to the experts! Thankya.

@jimidle
Copy link
Collaborator

jimidle commented Oct 31, 2022

@parrt We can merge this PR - it will not break anything as far as I can see.

@parrt parrt added this to the 4.11.2 milestone Oct 31, 2022
@parrt
Copy link
Member

parrt commented Oct 31, 2022

Thanks @jimidle !

@parrt parrt merged commit 5a68af0 into antlr:dev Oct 31, 2022
@parrt
Copy link
Member

parrt commented Oct 31, 2022

@perezd this should help the visitor, if you have time to check.

}

/** Not used for output; just used to distinguish between decl types
* to avoid dups.
*/
public String getArgType() { return ""; }; // assume no args

private boolean signature = false;
Copy link
Member

@KvanTTT KvanTTT Oct 31, 2022

Choose a reason for hiding this comment

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

If I understand correctly, it should be marked with final.

}

/** Not used for output; just used to distinguish between decl types
* to avoid dups.
*/
public String getArgType() { return ""; }; // assume no args

private boolean signature = false;
public boolean getSignature() { return signature; }
Copy link
Member

Choose a reason for hiding this comment

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

No need to use getter method if signature is final.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call: #3947

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants