Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace unnecessary validations with assertions #15185

Merged
merged 5 commits into from
Jun 13, 2024
Merged

Replace unnecessary validations with assertions #15185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b66df3e
CompilerStack: Remove some misapplied std:: prefixes from comments an…
cameel Jun 5, 2024
e2fb57b
CompilerStack: Convert overzealous input validations to assertions
cameel Jun 5, 2024
12e8b0c
Use solAssert() instead of throwing InternalCompilerError directly
cameel Jun 6, 2024
8d561e3
Assert presence of errors in case of FatalError instead of rethrowing
cameel Jun 6, 2024
2694190
Ignore warnings and infos when checking if FatalError was reported
cameel Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions libsolidity/analysis/NameAndTypeResolver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <boost/algorithm/string.hpp>
#include <unordered_set>

using namespace std::string_literals;
using namespace solidity::langutil;

namespace solidity::frontend
Expand Down Expand Up @@ -60,10 +61,9 @@ bool NameAndTypeResolver::registerDeclarations(SourceUnit& _sourceUnit, ASTNode
{
DeclarationRegistrationHelper registrar(m_scopes, _sourceUnit, m_errorReporter, m_globalContext, _currentScope);
}
catch (langutil::FatalError const&)
catch (langutil::FatalError const& error)
{
if (m_errorReporter.errors().empty())
throw; // Something is weird here, rather throw again.
solAssert(m_errorReporter.hasErrors(), "Unreported fatal error: "s + error.what());
Copy link
Member Author

@cameel cameel Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a bug though. hasErrors() is not the same as !errors.empty(). The latter will still count warnings and infos. This means we'll silence a buggy FatalError if some unrelated warning or info happened to be reported before it.

I don't think the results of this are visible to the user, since it only makes a difference when combined with another bug (throwing FatalError without reporting it), but nevertheless, it looks unintended.

return false;
}
return true;
Expand Down Expand Up @@ -135,10 +135,9 @@ bool NameAndTypeResolver::resolveNamesAndTypes(SourceUnit& _source)
return false;
}
}
catch (langutil::FatalError const&)
catch (langutil::FatalError const& error)
{
if (m_errorReporter.errors().empty())
throw; // Something is weird here, rather throw again.
solAssert(m_errorReporter.hasErrors(), "Unreported fatal error: "s + error.what());
return false;
}
return true;
Expand All @@ -151,10 +150,9 @@ bool NameAndTypeResolver::updateDeclaration(Declaration const& _declaration)
m_scopes[nullptr]->registerDeclaration(_declaration, false, true);
solAssert(_declaration.scope() == nullptr, "Updated declaration outside global scope.");
}
catch (langutil::FatalError const&)
catch (langutil::FatalError const& error)
{
if (m_errorReporter.errors().empty())
throw; // Something is weird here, rather throw again.
solAssert(m_errorReporter.hasErrors(), "Unreported fatal error: "s + error.what());
return false;
}
return true;
Expand Down
4 changes: 1 addition & 3 deletions libsolidity/codegen/ExpressionCompiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2952,9 +2952,7 @@ void ExpressionCompiler::setLValueFromDeclaration(Declaration const& _declaratio
else if (m_context.isStateVariable(&_declaration))
setLValue<StorageItem>(_expression, dynamic_cast<VariableDeclaration const&>(_declaration));
else
BOOST_THROW_EXCEPTION(InternalCompilerError()
<< errinfo_sourceLocation(_expression.location())
<< util::errinfo_comment("Identifier type not supported or identifier not found."));
solAssert(false, "Identifier type not supported or identifier not found.");
}

void ExpressionCompiler::setLValueToStorageItem(Expression const& _expression)
Expand Down
5 changes: 1 addition & 4 deletions libsolidity/codegen/LValue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,10 +441,7 @@ void StorageItem::storeValue(Type const& _sourceType, SourceLocation const& _loc
m_context << Instruction::SWAP1 << Instruction::POP;
}
else
BOOST_THROW_EXCEPTION(
InternalCompilerError()
<< errinfo_sourceLocation(_location)
<< util::errinfo_comment("Invalid non-value type for assignment."));
solAssert(false, "Invalid non-value type for assignment.");
}
}

Expand Down
Loading