Replace empty check of error, warning and info list with check for errors only #15203

matheusaaguiar · 2024-06-18T04:25:25Z

This is a minor bug found here by @cameel.
There are some instances where an intended check for errors can incorrectly consider warnings and info as well.

matheusaaguiar

There is also an instance of using errors().empty() check at

solidity/libsolidity/formal/ModelChecker.cpp

Line 142 in d0190e1

else if (!m_unsupportedErrorReporter.errors().empty())

but in this case it is specifically used to collect only warnings in SMTEncoder, BMC and CHC.

matheusaaguiar · 2024-06-18T04:28:42Z

libsolidity/interface/StandardCompiler.cpp

@@ -1647,7 +1647,7 @@ Json StandardCompiler::compileYul(InputsAndSettings _inputsAndSettings)
 	std::string const& sourceContents = _inputsAndSettings.sources.begin()->second;

 	// Inconsistent state - stop here to receive error reports from users
-	if (!stack.parseAndAnalyze(sourceName, sourceContents) && stack.errors().empty())
+	if (!stack.parseAndAnalyze(sourceName, sourceContents) && !stack.hasErrors())


Although there are no warnings in Yul Parser, if that happens to be the case in the future, this would already prevent the mistake of consider them as errors. Also, it follows the same logic of the changes made to instances of ErrorReporter::errors().

I mean, we have no warnings in AsmParser but we do have some in AsmAnalysis. So you can actually get a warning from parseAndAnalyze().

But anyway, this is a good change.

Actually, I'd go even further and make it an assert. That's what we do already in compileSolidity(). The exception will then be caught by the top-level handler in StandardCompiler::compile() and reported as error anyway.

This will affect the behavior slightly - the user will no longer get anything already added to output - but I think that's the correct behavior in case of a failed assumption anyway. I.e. drop everything and just fail immediately.

cameel · 2024-06-20T20:58:26Z

libsolidity/codegen/CompilerContext.cpp

@@ -473,7 +473,7 @@ void CompilerContext::appendInlineAssembly(
 			dialect,
 			identifierAccess.resolve
 		).analyze(*parserResult);
-	if (!parserResult || !errorReporter.errors().empty() || !analyzerResult)
+	if (!parserResult || errorReporter.hasErrors() || !analyzerResult)
 		reportError("Invalid assembly generated by code generator.");


I'd actually keep it at !errorReporter.errors().empty() here. This only runs on our own code produced by the codegen, we don't want warnings there.

Same for the other checks in this file, below.

By the way, maybe we should actually introduce a new helper with a very overt name, like hasErrorsWarningsOrInfos() to make this error harder to make in the future. Then just remove all uses of errors().empty() anyway.

Alternatively, we could rename errors() to reports(). But a helper would require fewer changes, so maybe it's a better option for now.

cameel · 2024-06-20T21:07:08Z

libsolidity/interface/StandardCompiler.cpp

@@ -1647,7 +1647,7 @@ Json StandardCompiler::compileYul(InputsAndSettings _inputsAndSettings)
 	std::string const& sourceContents = _inputsAndSettings.sources.begin()->second;

 	// Inconsistent state - stop here to receive error reports from users
-	if (!stack.parseAndAnalyze(sourceName, sourceContents) && stack.errors().empty())
+	if (!stack.parseAndAnalyze(sourceName, sourceContents) && !stack.hasErrors())


I mean, we have no warnings in AsmParser but we do have some in AsmAnalysis. So you can actually get a warning from parseAndAnalyze().

But anyway, this is a good change.

cameel · 2024-06-20T21:15:27Z

libsolidity/interface/StandardCompiler.cpp

@@ -1647,7 +1647,7 @@ Json StandardCompiler::compileYul(InputsAndSettings _inputsAndSettings)
 	std::string const& sourceContents = _inputsAndSettings.sources.begin()->second;

 	// Inconsistent state - stop here to receive error reports from users
-	if (!stack.parseAndAnalyze(sourceName, sourceContents) && stack.errors().empty())
+	if (!stack.parseAndAnalyze(sourceName, sourceContents) && !stack.hasErrors())


Actually, I'd go even further and make it an assert. That's what we do already in compileSolidity(). The exception will then be caught by the top-level handler in StandardCompiler::compile() and reported as error anyway.

This will affect the behavior slightly - the user will no longer get anything already added to output - but I think that's the correct behavior in case of a failed assumption anyway. I.e. drop everything and just fail immediately.

cameel · 2024-06-20T21:17:13Z

test/libsolidity/InlineAssembly.cpp

 	if (_canWarn)
 		BOOST_REQUIRE(!Error::containsErrors(stack.errors()));
 	else
-		BOOST_REQUIRE(stack.errors().empty());
+		BOOST_REQUIRE(!stack.hasErrors());


This one was intentional. Note the _canWarn variable. If we're in the else branch, we don't expect any warnings.

cameel · 2024-06-20T21:26:32Z

test/libsolidity/InlineAssembly.cpp

@@ -228,7 +228,7 @@ BOOST_AUTO_TEST_CASE(print_string_literal_unicode)
 		DebugInfoSelection::None()
 	);
 	BOOST_REQUIRE(stack.parseAndAnalyze("", source));
-	BOOST_REQUIRE(stack.errors().empty());
+	BOOST_REQUIRE(!stack.hasErrors());


I think we don't want warnings or infos here.

cameel · 2024-06-20T21:27:29Z

test/libyul/Common.cpp

-	if (!stack.parseAndAnalyze("", _source) || !stack.errors().empty())
+	if (!stack.parseAndAnalyze("", _source) || stack.hasErrors())
 		BOOST_FAIL("Invalid source.");


Same here. I think we don't want warnings or infos here.

cameel · 2024-06-20T21:31:34Z

test/tools/yulrun.cpp

@@ -65,7 +65,7 @@ std::pair<std::shared_ptr<Block>, std::shared_ptr<AsmAnalysisInfo>> parse(std::s
 	);
 	if (stack.parseAndAnalyze("--INPUT--", _source))
 	{
-		yulAssert(stack.errors().empty(), "Parsed successfully but had errors.");
+		yulAssert(!stack.hasErrors(), "Parsed successfully but had errors.");


Would be best to check how it behaves on input that generates warnings. Try this:

{ let x := 42 switch x default { sstore(x, x) } }

Also, to print those warnings.

matheusaaguiar added 2 commits June 18, 2024 01:18

Replace errors/warning/info empty check with proper errors only check

874b1d4

Add hasErrors to YulStack and replace errors list empty check

5873ce1

matheusaaguiar self-assigned this Jun 18, 2024

matheusaaguiar commented Jun 18, 2024

View reviewed changes

matheusaaguiar marked this pull request as ready for review June 18, 2024 04:42

matheusaaguiar added the bug 🐛 label Jun 18, 2024

matheusaaguiar mentioned this pull request Jun 18, 2024

Catch unimplementedFeatureErrors and report them as errors #15168

Merged

cameel reviewed Jun 20, 2024

View reviewed changes

Use helper instead of errors().empty()

19b414c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace empty check of error, warning and info list with check for errors only #15203

Replace empty check of error, warning and info list with check for errors only #15203

matheusaaguiar commented Jun 18, 2024

matheusaaguiar left a comment

matheusaaguiar Jun 18, 2024

cameel Jun 20, 2024

cameel Jun 20, 2024

cameel Jun 20, 2024 •

edited

Loading

cameel Jun 20, 2024 •

edited

Loading

cameel Jun 20, 2024

cameel Jun 20, 2024

cameel Jun 20, 2024

cameel Jun 20, 2024

cameel Jun 20, 2024

cameel Jun 20, 2024

Replace empty check of error, warning and info list with check for errors only #15203

Are you sure you want to change the base?

Replace empty check of error, warning and info list with check for errors only #15203

Conversation

matheusaaguiar commented Jun 18, 2024

matheusaaguiar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

cameel Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Jun 20, 2024 •

edited

Loading

cameel Jun 20, 2024 •

edited

Loading