Catch unimplementedFeatureErrors and report them as errors

[matheusaaguiar]

First attempt on partially closing #15139.
This is focusing on making #15001 pass our CI tests and hopefully will serve as a base for future tasks of the issue mentioned before.

[cameel]

That one was really tough to review. Not because of what the PR itself does but because the way error handling is done in the compiler is just too messy, inconsistent and slightly wrong in many annoying ways. I had a really hard time giving suggestions here on how to do it properly without just saying to rewrite it all :)

In any case, this needs some changes, but the fundamental approach is good. I like the idea of making parse(), analyze() and compile() responsible for catching unimplemented feature errors rather than doing it outside.

I'd actually move even more handlers in there (e.g. YulException and CompilerError should IMO be handled inside compile()), but that's out of the scope of the PR so let's not do that here. But overall, I think that all exceptions that are considered a normal part of the compiler operation and are not a result of a bug should be handled inside the respective "stack" class. Everything else that escapes should then just bubble up to the top-level handler.

[cameel]

Still a few things to clean up.

[cameel]

Suggested change

	if (m_errorReporter.errors().empty())
	if (!m_errorReporter.hasErrors())

Oh, this is actually a minor bug (not in the PR - it was here already). Checking errors().empty() will count warnings and infos as errors, which is wrong - those should not fail compilatiohn. Turns out we don't have any warnings in Yul parser though, which is probably why it went unnoticed, but we should fix it nevertheless.

I see there are overall 16 uses of errors().empty() in the codebase and some of them might be intended but some look like bugs. Maybe it would be better to fix them in a separate PR. At least this one had no user-visible effects (and the fix does not require a changelog entry) but others might have.

[matheusaaguiar]

Fixed.

[cameel]

This is something you should at least put in a separate commit with its own description. It shouldn't be mixed in with other, unrelated changes, especially in a refactor like this. Like I said, I'd even go as far as to leave it as is here and only fix it in a separate PR along with the other instances.

[matheusaaguiar]

Ah right, I was thinking about checking the other cases later in another PR. I removed this change and will leave it for this future PR.

[matheusaaguiar]

PR created.

[cameel]

Two more things:

• You should wrap YulStack::optimize() in a handler too.
• This needs a changelog entry. It's either a bugfix or a minor compiler feature: unimplemented features are now properly reported as errors instead of being handled as if they were bugs.

[matheusaaguiar]

@cameel, I added a changelog and addressed your comments.

You should wrap YulStack::optimize() in a handler too.

It was already wrapped... or are you referring to YulStack::optimize(Object& _object, bool _isCreation) ? This latter one is private and only called by YulStack::optimize().

[cameel]

You should wrap YulStack::optimize() in a handler too.

It was already wrapped... or are you referring to YulStack::optimize(Object& _object, bool _isCreation) ? This latter one is private and only called by YulStack::optimize().

Ah, you're right! I completely forgot that we decided to do this in the top-level optimize() because this one is recursive.

[cameel]

There are two remaining things to resolve, but they're minor enough that I'm already approving the PR as a whole.

[cameel]

This is something you should at least put in a separate commit with its own description. It shouldn't be mixed in with other, unrelated changes, especially in a refactor like this. Like I said, I'd even go as far as to leave it as is here and only fix it in a separate PR along with the other instances.

[matheusaaguiar]

@cameel, I addressed your last review and squashed the commits.
Please approve again.