Refactor `compileViaYul` test setting #15819

rodiazet · 2025-02-03T16:56:38Z

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

~~Depends on: #15666~~ Merged

github-actions · 2025-02-03T16:56:50Z

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 · 2025-02-03T17:20:42Z

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

rodiazet · 2025-02-04T10:49:38Z

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

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

rodiazet · 2025-02-04T11:04:52Z

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

test/libsolidity/SemanticTest.h

cameel · 2025-02-12T16:08:14Z

test/libsolidity/SemanticTest.cpp

+	std::string bytecodeFormatString = m_reader.stringSetting("bytecodeFormat", "unset");
+	if (bytecodeFormatString == ">=EOFv1" && compileViaYul == CompileViaYul::False)
+		BOOST_THROW_EXCEPTION(std::runtime_error("Compilation to EOF requires using Yul IR"));


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.

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"}.

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.

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

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 :)

test/libsolidity/SemanticTest.h

test/TestCase.h

test/TestCase.cpp

test/libsolidity/SyntaxTest.cpp

test/libsolidity/SemanticTest.cpp

test/libsolidity/SyntaxTest.cpp

test/TestCase.cpp

tests: Use enum setting for `compileViaYul`

cameel

Looks good, but needs some final cleanup.

I'd also try to just remove compileViaYul from two of the tests. They probably work on both codegens now.

cameel · 2025-03-14T18:11:55Z

test/TestCase.cpp

 		m_shouldRun = false;
-	else if (bytecodeFormatString == ">=EOFv1" && !eofVersion.has_value())
+	else if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && !eofVersion.has_value())


To clarify, this is what I meant by "same for the legacy run" in #15819 (comment):

Suggested change

else if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && !eofVersion.has_value())

else if (!bytecodeFormat().contains(BytecodeFormat::Legacy) && !eofVersion.has_value())

cameel · 2025-03-14T18:13:37Z

test/libsolidity/SyntaxTest.cpp

+	solUnimplementedAssert(m_compileViaYul != CompileViaYul::Also,
+		"'compileViaYul: also' is not yet supported in syntax tests.");


One left from #15819 (comment):

Suggested change

solUnimplementedAssert(m_compileViaYul != CompileViaYul::Also,

"'compileViaYul: also' is not yet supported in syntax tests.");

if (m_compileViaYul == CompileViaYul::Also)

solUnimplemented("'compileViaYul: also' is not yet supported in syntax tests.");

cameel · 2025-03-14T20:34:14Z

test/libsolidity/SyntaxTest.cpp

+	if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && m_compileViaYul == CompileViaYul::False)
+		BOOST_THROW_EXCEPTION(std::runtime_error("Compilation to EOF requires using Yul IR"));


Still needs an assert.

While at it, let's also make the message a bit clearer. We should mention which settings it's about because the error won't point at a specific line in the test.

Suggested change

if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && m_compileViaYul == CompileViaYul::False)

BOOST_THROW_EXCEPTION(std::runtime_error("Compilation to EOF requires using Yul IR"));

if (m_compileViaYul == CompileViaYul::False)

{

soltestAssert(!eofEnabled);

if (bytecodeFormat().contains(BytecodeFormat::EOFv1))

BOOST_THROW_EXCEPTION(std::runtime_error(

"Conflicting test settings: 'bytecodeFormat' includes EOF, which requires compilation via IR, but 'compileViaYul' is set to false."

));

}

cameel · 2025-03-14T20:36:12Z

test/libsolidity/SemanticTest.cpp

+	if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && compileViaYul == CompileViaYul::False)
+		BOOST_THROW_EXCEPTION(std::runtime_error("Compilation to EOF requires using Yul IR"));
+
+	if (compileViaYul == CompileViaYul::False && eofEnabled)
 		m_shouldRun = false;


The eofEnabled check should be converted to an assert, like in the syntax test. And we adjust the message the same way:

Suggested change

if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && compileViaYul == CompileViaYul::False)

BOOST_THROW_EXCEPTION(std::runtime_error("Compilation to EOF requires using Yul IR"));

if (compileViaYul == CompileViaYul::False && eofEnabled)

m_shouldRun = false;

if (m_compileViaYul == CompileViaYul::False)

{

soltestAssert(!eofEnabled);

if (bytecodeFormat().contains(BytecodeFormat::EOFv1))

BOOST_THROW_EXCEPTION(std::runtime_error(

"Conflicting test settings: 'bytecodeFormat' includes EOF, which requires compilation via IR, but 'compileViaYul' is set to false."

));

}

cameel · 2025-03-14T20:42:59Z

test/libsolidity/semanticTests/calldata/calldata_array_dynamic_bytes.sol

@@ -67,6 +67,7 @@ contract C {

 // ====
 // compileViaYul: false
+// bytecodeFormat: legacy


// via yul disabled because of stack issues.

Can you try to remove compileViaYul: false here? The code does compile on 0.8.29 for me. If it still fails, I'd also try onlyOnEOF.

Same for negative_stack_height.sol.

github-actions bot added the external contribution ⭐ label Feb 3, 2025

cameel mentioned this pull request Feb 3, 2025

eof: Enable compilation to EOF for all EVMVersionRestrictedTestCase tests by default #15666

Merged

cameel added testing 🔨 EOF labels Feb 3, 2025

cameel changed the title ~~Refactor compile via yul flag~~ Refactor compileViaYul test setting Feb 3, 2025

rodiazet force-pushed the refactor-compile-via-yul-flag branch from 61b065e to 36bb4e8 Compare February 4, 2025 11:03

rodiazet force-pushed the refactor-compile-via-yul-flag branch from 36bb4e8 to cf8ed7c Compare February 7, 2025 14:31

cameel reviewed Feb 12, 2025

View reviewed changes

rodiazet force-pushed the refactor-compile-via-yul-flag branch 6 times, most recently from 884cc27 to b53af39 Compare February 20, 2025 13:44

cameel reviewed Mar 7, 2025

View reviewed changes

rodiazet force-pushed the refactor-compile-via-yul-flag branch from b53af39 to e39a5a6 Compare March 12, 2025 15:02

cameel reviewed Mar 14, 2025

View reviewed changes

test/libsolidity/SemanticTest.cpp Outdated Show resolved Hide resolved

test/libsolidity/SyntaxTest.cpp Outdated Show resolved Hide resolved

test/TestCase.cpp Outdated Show resolved Hide resolved

test/TestCase.cpp Outdated Show resolved Hide resolved

rodiazet added 3 commits March 14, 2025 10:29

eof: Add semantic test setting consistency check.

bc81820

tests: Use enum setting for `compileViaYul`

Address review comments

aa3a789

Address review comments

3e5c39c

rodiazet force-pushed the refactor-compile-via-yul-flag branch from e39a5a6 to 3e5c39c Compare March 14, 2025 09:29

cameel reviewed Mar 14, 2025

View reviewed changes

cameel mentioned this pull request Apr 19, 2025

New test setting types: enum list/enum set #16015

Draft

	else if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && !eofVersion.has_value())
	else if (!bytecodeFormat().contains(BytecodeFormat::Legacy) && !eofVersion.has_value())

		solUnimplementedAssert(m_compileViaYul != CompileViaYul::Also,
		"'compileViaYul: also' is not yet supported in syntax tests.");

		if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && m_compileViaYul == CompileViaYul::False)
		BOOST_THROW_EXCEPTION(std::runtime_error("Compilation to EOF requires using Yul IR"));

-	if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && m_compileViaYul == CompileViaYul::False)
-		BOOST_THROW_EXCEPTION(std::runtime_error("Compilation to EOF requires using Yul IR"));
+	if (m_compileViaYul == CompileViaYul::False)
+	{
+		soltestAssert(!eofEnabled);
+		if (bytecodeFormat().contains(BytecodeFormat::EOFv1))
+			BOOST_THROW_EXCEPTION(std::runtime_error(
+				"Conflicting test settings: 'bytecodeFormat' includes EOF, which requires compilation via IR, but 'compileViaYul' is set to false."
+			));
+	}

Refactor compileViaYul test setting #15819

Are you sure you want to change the base?

Refactor compileViaYul test setting #15819

Uh oh!

Conversation

rodiazet commented Feb 3, 2025 • edited by cameel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 3, 2025

Uh oh!

cameel commented Feb 3, 2025

Uh oh!

rodiazet commented Feb 4, 2025

Uh oh!

rodiazet commented Feb 4, 2025

Uh oh!

Uh oh!

cameel Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cameel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Refactor `compileViaYul` test setting #15819

Refactor `compileViaYul` test setting #15819

rodiazet commented Feb 3, 2025 •

edited by cameel

Loading

cameel Feb 12, 2025 •

edited

Loading

rodiazet Feb 14, 2025 •

edited

Loading

rodiazet Feb 14, 2025 •

edited

Loading