%grmtools section lacking warnings_are_errors flag

[ratmice]

This is relatively minor and isn't clear to me if this is actually a bug or should just be closed.
The workaround is to just set it in CTParserBuilder, there is no equivalent for nimbleparse though
so the only thing missing is the ability to force nimbleparse to treat warnings as errors.

There isn't exactly a direct RTParserBuilder equivalent of warnings_are_errors method of CTParserBuilder.

There is a question, about whether it is possible that this behavior can be configured by the parser or not. Or whether this is a behavior that e.g. nimbleparse would extract the key from the header and implement on it's own. It appears to me that YaccGrammarError, and YaccGrammarWarning are disjoint in that you couldn't get ASTWithValidityInfo::errors() to return a YaccGrammarError containing a YaccGrammarWarning. This from a library perspective then means the key would not be easily applied to downstream consumers using RTParserBuilder directly.

So maybe this isn't something that should be contained in the %grmtools section?

List of relevant methods/types.

• CTParserBuilder::warnings_are_errors
• ASTWithValidityInfo::errors
• GrammarAST::warnings
• YaccGrammarError
• YaccGrammarWarning

[ratmice]

So I did have a look at and indeed, the conversion from warning to error happens very late in the Box<dyn Error> return value of CTParserBuilder around

grmtools/lrpar/src/lib/ctbuilder.rs

Lines 628 to 660 in bbb0343

	let grm = match res {
	Ok(_) if self.warnings_are_errors && !warnings.is_empty() => {
	let mut out = String::new();
	out.push_str(&format!(
	"\n{ERROR}{}\n",
	yacc_diag.file_location_msg("", None)
	));
	for e in warnings {
	out.push_str(&format!(
	"{}\n",
	indent(" ", &yacc_diag.format_warning(e).to_string())
	));
	}
	return Err(ErrorString(out))?;
	}
	Ok(grm) => {
	if !warnings.is_empty() {
	for w in warnings {
	let ws_loc = yacc_diag.file_location_msg("", None);
	let ws = indent(" ", &yacc_diag.format_warning(w).to_string());
	// Assume if this variable is set we are running under cargo.
	if std::env::var("OUT_DIR").is_ok() && self.show_warnings {
	for line in ws_loc.lines().chain(ws.lines()) {
	println!("cargo:warning={}", line);
	}
	} else if self.show_warnings {
	eprintln!("{}", ws_loc);
	eprintln!("{WARNING} {}", ws);
	}
	}
	}
	grm
	}

. So I'm not sure it's really worth the effort trying to make it work within the %grmtools section itself.

I think the way if we wanted to try would be a new YaccGrammarError::Warning(YaccGrammarWarning) variant, but otherwise we might just close as not planned?

[ltratt]

Good question. Implicitly at the moment we say to the consumer (ctbuilder, nimbleparse, etc) "you choose whether you want warnings to be errors or not". We could make the core of lrpar take that on itself, but there is one question: at what point do we say "warnings are now errors"? I think we'd basically have to do it at the exit point(s) (i.e. when we've already collected all the errors and warnings, and we could then merge the two), which in lrpar would probably mean when ASTWithValidityInfo is created? This seems doable to me, but I haven't actually looked deeply!

[ratmice]

I think when ASTWithValidityInfo is created is right, so currently you can get warnings via ASTWithValidityInfo::ast().warnings(), Unfortunately those are derived lazily from the AST,
we'd probably need to add a GrammarAST::warnings field, so we can move them into the ASTWithValidityInfo::errs field. (Edit: I think perhaps just setting a flag with a private function to indicate that warnings were already consumed after which warnings() returns an empty vec could be easiest)

ASTWithValidityInfo constructors might be feasible, but there are two:
ASTWithValidityInfo::new(YaccKind) which is currently %grmtools section oblivious and predates the grmtools section. But there is also ASTWithValidityInfo::from_str() which parses the section to get the YaccKind.

So if we do it from those constructors, we likely need to parse the header from both those constructors, adding the new flag to from_str and some new section parsing code for new.

We could potentially do it during complete_and_validate() since it would then cover both those constructors. But we need to figure out how to get the contents of the %grmtools section, and it has been dropped by then. I think it would be the ideal place if we could figure that part out.