Refactor compileViaYul test setting

[rodiazet]

This PRs refactors using of compileViaYul setting in semantic tests with using enumSetting

Depends on: #15666 Merged

[github-actions]

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

[cameel]

Please also add the missing validation you found in #15666 (comment).

[rodiazet]

Please also add the missing validation you found in #15666 (comment).

Ok. Do you want me to introduce similar change in SyntaxTest?

[rodiazet]

Please also add the missing validation you found in #15666 (comment).

Ok. Do you want me to introduce similar change in SyntaxTest?

Added but I think we should also use enumSetting for bytecodeFormat and add a protected member to the EVMVersionRestrictedTestCase class to avoid parsing this setting twice

[cameel]

I think we should also use enumSetting for bytecodeFormat and add a protected member to the EVMVersionRestrictedTestCase class to avoid parsing this setting twice

Agreed, at least regarding exposing it on EVMVersionRestrictedTestCase. But I'd expose it via a getter + private member to prevent accidental modification.

Checking on the raw setting technically requires reparsing the value again. It only works here without it because parsing is trivial right now (we hard-coded it for a few values) but this will change at some point.

[cameel]

As for enumSetting, it's not the right type. The value in a general case is a list of ranges of values from an enumeration. Once fully implemented, it should be able to handle individual values, values with operators (like = or >=), and maybe even things like A-B or A..B. The EVMVersion is the same kind of setting (just with different enumeration values) and there are a few other settings where this would be useful too.

But that's a bit more refactoring than I was expecting here, so unless you really want to do it (I wouldn't mind :)), IMO it would be enough to just keep it as a string setting and just expose the parsed value on EVMVersionRestrictedTestCase as a set of enum values. E.g. legacy,>=EOFv1 should be parsed into {"legacy", "EOFv1"}.

[rodiazet]

Agreed, at least regarding exposing it on EVMVersionRestrictedTestCase. But I'd expose it via a getter + private member to prevent accidental modification.

But unfortunately we cannot do it like this b/c reading this flag in Semantic test with default unset is crucial here. We need to know if the flag was set or not. In case of moving this to EVMVersionRestrictedTestCase members we don't have this inforamation.

[rodiazet]

There is another problem here. When I added compileViaYul and bytecodeForamat flags consistency check to SyntaxTests some tests started failing. The reason is that in these tests compileViaYul flag is not set and (when compiling to legacy) the default is false. It's similar issue that we don't know at this point if the default was taken or not. I removed it for now but I think it needs refactoring of enumSetting to additionally inform if the default was taken.

It works for SemanticTest b/c the default in this case is Also

[cameel]

But unfortunately we cannot do it like this b/c reading this flag in Semantic test with default unset is crucial here.

If I understand correctly, you want this because you don't want the validation to run when bytecodeFormat is not set explicitly? Why is that necessary? With the default being legacy,>=EOFv1 currently the validation will always pass anyway.

And I think that in general things are simpler when you can assume that not providing an explicit value is exactly equivalent to using the default value. If the default needs a special behavior, it's always possible to achieve by defining a separate value for that behavior and using that as default.

The reason is that in these tests compileViaYul flag is not set and (when compiling to legacy) the default is false.

Right. So this seems to be one of those cases where we need such a special value. Currently we use different defaults based on the value of --eof-version, but we can just as well explicitly add a value like compileViaYul: onlyOnEOF and set default in syntax tests to that.

Another option, would be to keep the dynamic default, but set it based on the value of bytecodeFormat setting rather than --eof-version. We'd set the default to true when bytecodeFormat includes any EOF version (regardless of whether it also includes legacy or not). This would give us slightly different behavior (legacy tests that need to also work on EOF would now always compile via IR) but that would still be acceptable.

I think I'd go with the first option (onlyOnEOF) personally. I initially suggested those dynamic defaults, because they seemed to be better than the alternative, but maybe that wasn't the best idea. We've already hit problems due to having layers of conditional defaults in optimizer settings (#15641) and I'd rather avoid creating a similar mess here :)

[cameel]

Ok. Do you want me to introduce similar change in SyntaxTest?

Yes, please. They should both work the same way.

Just note that also is currently missing in SyntaxTest. It's fine to add it there and use solUnimplemented() to disable it.