Enforce that disambiguation functions returning one of the terminals from the set they are disambiguating

[krame505]

Currently a value of any type may be plucked. This is an error, as only terminals in disambiguation functions and terminal identifiers in disambiguation classes should be pluckable.

[602p]

The easy part of this is enforcing that the type passed to pluck is a terminal identifier (I have a patch that does this that I'll PR shortly.) This solves the nasty cases that pluck 3 would compile, and that pluck "foo" would only get caught by javac.

The harder part is making sure that it's one of the terminals being disambiguated amongst - that might deserve it's own issue, or could co-opt this one. I'm not sure what a good solution to that is.

There's a hacky approach I found suggested in a comment by @tedinski , which is to bind over the names of the legal terminal identifiers with some marking type, and enforce that type when typechecking pluck. I tried implementing this using a newSkolemConstant() per action block as the marker type. This seems to work-ish for the restricted case of disambiguation functions. It's a hacky solution though, and I don't think it solves the more challenging case when disambiguation functions pluck terminal identifiers from parser attributes.

In that case we need to enforce that all assignments to that attribute are legal terminals to be plucked in the disambiguation function. Additionally, it is currently legal to have multiple disambiguation functions pluck from the same parser attribute, since as long as the attribute only ever holds terminals in the intersection of the groups being disambiguated this works fine.

As a short term change, we might consider adding runtime checks to make sure the returned terminal identifier is amongst the legal ones.

[krame505]

That is more or less the situation as it stands. We do have runtime checking (in Copper) for disambiguation functions and classes to ensure that the plucked terminal is currently in the set of shiftable terminals - failing this currently raises a syntax error, I think. But ensuring that the plucked terminal of a disambiguation class is actually in the class is a tough problem using types - really the only nice way of handling this (and the similar case of disambiguation functions) is to have type classes, with an type class automatically defined for each disambiguation function/class.
For now I would recommend only checking that what is plucked is a TerminalId, and leave function/class membership to be checked at runtime, until we have added type classes to Silver.

[602p]

I was poking around in Copper, and can't find such a check. The silver code

terminal Id_t /[a-zA-Z][a-zA-Z0-9]*/;
terminal IntLit_t /[0-9]+/;
terminal If_t /if/;
ignore terminal WhiteSpace_t /[\t\r\n\ ]+/  ;

disambiguate Id_t, If_t {
  pluck IntLit_t;
}

produces a parser that will parse "if" as an IntLit_t. Complete example as a Gist.

I have a patch I can PR against Copper to add such a check to the site in SingleDFAEngine where we call runDisambiguationAction. Does it belong there, or in the code we generate for runDisambiguationAction/disambiguate_XXX?

[krame505]

I'll try to take a look tonight. Are you 100% sure the disambiguation function is actually getting called - maybe try adding a print statement? I don't know that could run without some sort of error. Lucas Kramer

On Wed, May 29, 2019 at 11:04 AM Louis Goessling [email protected] wrote:

I was poking around in Copper, and can't find such a check. The silver code

terminal Id_t /[a-zA-Z][a-zA-Z0-9]*/; terminal IntLit_t /[0-9]+/; terminal If_t /if/; ignore terminal WhiteSpace_t /[\t\r\n\ ]+/ ;

disambiguate Id_t, If_t { pluck IntLit_t; }

produces a parser that will parse "if" as an IntLit_t. Complete example as a Gist. https://gist.github.com/602p/3403f64fe9b264018a04d694edf049f8

I have a patch https://gist.github.com/602p/461723eb0d6f59bd73063b68ce4f762a I can PR against Copper to add such a check to the site in SingleDFAEngine where we call runDisambiguationAction. Does it belong there, or in the code we generate for runDisambiguationAction/disambiguate_XXX?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/silver/issues/304?email_source=notifications&email_token=ACATLCWYTRWSA3VYFLVV7I3PX3ATHA5CNFSM4GS24BWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWQFIOI#issuecomment-497046585, or mute the thread https://github.com/notifications/unsubscribe-auth/ACATLCSMJWG2ITQJ7KKX4WDPX3ATHANCNFSM4GS24BWA .

[602p]

Note for the future: I've opened a issue on Copper about the lack of a runtime check as mentioned. This issue is now tracking the "other half" of the original problem: that we don't have a way to check this statically in Silver.