proposal: x/tools/cmd/goyacc: reduce dependence on globals

[SteelPhase]

background

Code generated by goyacc has a dependence on package globals that it doesn't need to have.

yyDebug, and yyErrorVerbose do not specifically need to be globals, and can be variables on the parser instead.

I did not find documentation for bison/yacc that states YYDEBUG or a YYERRORVERBOSE need to be made available inside grammar actions.

We can follow java here and expose them via the Parser which is already technically available to grammar actions but not documented.

description

I'm proposing the following

• Move yyDebug, and yyErrorVerbose into yyParserImpl
• For compatibility, modify yyNewParser to grab the value of the package globals, and set them internally when called
  • something like `return &$$ParserImpl{ debug: $$Debug, errorVerbose: $$ErrorVerbose}
• Create a new yyNewParser### function that exposed debug, and errorVerbose as inputs
• Create a new yyParse### function that exposed debug, and errorVerbose as inputs
• Modify the yyParser interface to expose, and allow modification of, the internal state of debug, and errorVerbose
• Modify yyParserImpl to match the updated yyParser interface

drawbacks

There is potential that if someone had already written a customer parser to match yyParser their parser would no longer match the interface

Any grammar actions that depend on yyDebug or yyErrorVerbose would break. A key point here is this behavior doesn't seem to be well defined anyways, and I'm not sure if the intention was to make them available in grammar actions.

[gopherbot]

Change https://golang.org/cl/298389 mentions this issue: goyacc: reduce dependence on globals

[rogpeppe]

I'm not entirely certain that this is worth doing. I know it's not totally ideal that these settings are global, but I'd be interested to hear about an actual scenario where the settings need to vary dynamically for a given grammar.

Specifically:

• yyDebug seems like something that you'll only want to enable when debugging the grammar during development.
• yyErrorVerbose seems like something that would be reasonable to make a one-time decision for, rather than dynamically changing it.

[SteelPhase]

In my case I have other structs that rely on the generated parser at runtime. I've exposed the ability to change these settings at that level, but because the values are global, I can't guarantee that 1 instance wouldn't clobber another.

[rogpeppe]

I've exposed the ability to change these settings at that level

Perhaps you should not have done that? ISTM that it might be better to choose those values statically when generating the code, or to expose them as globals.

I'm still interested to hear a concrete use case for why these settings would really benefit from being dynamically settable - i.e. why would the clients of your library want to do this?

[SteelPhase]

I had two requirements for this. I have a path that is meant to be as performant as possible, and a path for gathering simulation, and validation information. I need these settings in both of these states for either path. I'm pretty sure there is no work around. I've gone ahead and pulled a copy of goyacc, into my project, and made the changes I needed to, for this to work.

If no one else sees getting rid of these globals as a benefit it doesn't need to be pulled into golang/tools.

[rogpeppe]

What I don't understand is why you might need both of those paths to be enabled concurrently.

[SteelPhase]

I really don't want to get into the weeks of the project, but in this scenario, these two paths can be used by the same project. The current cause of this strive was caused by an api that offers endpoints that each need one or the other. One for performance reason, and the other for diagnostics.