Skip to content
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

parse() throws am ambiguity exception even after an action has eliminated the amb node #1210

Open
eh-adblockplus-org opened this issue Aug 11, 2018 · 3 comments

Comments

@eh-adblockplus-org
Copy link

Test t1 fails and not because of an assertion failure. The example is a model of resolving ambiguities by a strict priority order (not, however, with a priority rule). Production A is always preferred over B; the example has no other possibilities, but that's an entailment of simplicity and not essential. The action rewrites the ambiguity set to be just the A element.
The defect is that parsing without allowing ambiguity still throws an Ambiguity, even though there's no ambiguity node in what would otherwise be the parse tree. If there were no defect, t1 would pass. As it is now, the second parse, which allows ambiguity so that we get a parse tree to examine, returns a tree that does not contain an ambiguity node.
A bit of time in the debugger shows that the action is being executed twice, once for each parse. Apparently the defect, therefore, arises from not considering the effect of an action that rewrites an ambiguity into an ordinary application.

module amb_01

import ParseTree ;

syntax X = a: A | b: B ;
syntax A = "X" ;
syntax B = "X" ;
// Match a concrete pattern to have an X as return value.
X amb({f:(X)`<A _>`, *value _}) = f ;

test bool t1()
{
	try
	{
		Tree t = parse( #X, "X", allowAmbiguity=false ) ;
		return true ;
	}
	catch Ambiguity( loc _, str _, str _ ):
	{
		Tree t = parse( #X, "X", allowAmbiguity=true ) ;
		assert /amb(_) !:= t : "Tree still contains an ambiguity node." ;
		return false ;
	}
}
@jurgenvinju
Copy link
Member

good find. It may be the *value _ that throws off the implementation of the post-parse filter. I'll have a look.

@jurgenvinju
Copy link
Member

Ok, complex issue. The post-parse filtering only works if allowAmbiguity=false and that is contra-intuitive.

  • reason: when allowAmbiguity=false no parse forest is constructed at all on which the amb rewrite rule {w,c,sh}ould match
  • Either, The name allowAmbiguity may be misleading here,
  • Or the implementation of allowAmbiguity=false should look to see if a rewrite rule would filter the node anyway before complaining about the ambiguity.

@jurgenvinju
Copy link
Member

jurgenvinju commented Dec 30, 2018

	Object parse(String nonterminal, URI inputURI, char[] input, IActionExecutor<T> actionExecutor, INodeFlattener<T, S> converter, INodeConstructorFactory<T, S> nodeConstructorFactory, IRecoverer<P> recoverer);
  1. the action executor is supplied independently to the parser fromt the node constructor factory
  2. the action executor is executed after a node is constructed on the node
  3. but the node constructor factory throws an ambiguity exception already before the action can be executed due to allowAmbiguity=false

So this interaction requires re-thinking of the design of three components. I'm parking this for now, since we're planning on integrating the data-dependent context-free grammar parsing framework Iguana this year, and we'll have to reconsider the post-parse actions in that regard anyway.

Initial thoughts:

  • With data-dependent parsing (under the hood) it could be argued that post-parse filtering has become less important
  • normalizing rewrite rules on all constructors should work anyway, so post-parse filtering is actually just a function of this normalization feature
  • a different implementation of the normalization feature, not wired into the parser would be nicer.
  • but this still interacts with the allowAmbiguity feature which would cry havoc before the normalizer has removed the ambiguity.

Ergo, this is something to think deeper about.

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

No branches or pull requests

2 participants