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.

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

Stop after parsing. #9364

Merged
merged 2 commits into from
Sep 30, 2020
Merged

Stop after parsing. #9364

merged 2 commits into from
Sep 30, 2020

Conversation

chriseth
Copy link
Contributor

@chriseth chriseth commented Jul 8, 2020

Fixes #8739

Todo:

  • error out if binary was requested in combination with "stop after parsing"
  • implement this in standard-io

@chriseth
Copy link
Contributor Author

chriseth commented Jul 8, 2020

Would be deligted if someone took this over :)

@Marenz
Copy link
Contributor

Marenz commented Jul 21, 2020

I am taking this over :)

@Marenz
Copy link
Contributor

Marenz commented Jul 22, 2020

Pushed an update. this is still WIP, but feel free to give feedback on the current state.

@@ -3494,7 +3497,7 @@ string FunctionType::externalSignature() const
}

// "inLibrary" is only relevant if this is not an event.
bool const inLibrary = kind() != Kind::Event && dynamic_cast<ContractDefinition const&>(*m_declaration->scope()).isLibrary();
bool const inLibrary = kind() != Kind::Event && m_declaration->scope() && dynamic_cast<ContractDefinition const&>(*m_declaration->scope()).isLibrary();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering whether we should rather assign scopes during parsing - it should be a purely syntactic thing, so the parser should be able to do that easily.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally or for this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally.

Copy link
Member

Choose a reason for hiding this comment

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

Just because I'm randomly reading these comments: I actually thought the same when looking at #8834 - the m_free member there could just be derived from the scope, if the parser did this stuff already...

@Marenz
Copy link
Contributor

Marenz commented Jul 23, 2020

Updated: now missing is the documentation update

vector<string> contractNames;
for (auto const& contract: m_contracts)
contractNames.push_back(contract.first);

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if I maybe should make this return value based on the imports instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what you are suggesting here, but do you think it makes sense to just keep the code here and instead initialize m_contracts already right after parsing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I removed this change completely now. Turns out it was useless.

@Marenz Marenz force-pushed the stopafter branch 3 times, most recently from f9f3e11 to 0a24a29 Compare July 23, 2020 13:01
@Marenz Marenz self-assigned this Jul 27, 2020
@Marenz
Copy link
Contributor

Marenz commented Jul 27, 2020

Documentation is also updated now

Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

Needs to be rebased and changelog moved to 0.7.1.

@@ -731,7 +736,7 @@ bool ASTJsonConverter::visit(FunctionCall const& _node)
attributes.emplace_back("isStructConstructorCall", _node.annotation().kind == FunctionCallKind::StructConstructorCall);
attributes.emplace_back("type_conversion", _node.annotation().kind == FunctionCallKind::TypeConversion);
}
else
else if (_node.annotation().kind != FunctionCallKind::Unset)
Copy link
Member

Choose a reason for hiding this comment

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

Is this a bugfix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it fixes a bug that happened as a result from the stop-after-parse change. I think usually you wouldn't get Unset.

solc/CommandLineInterface.cpp Outdated Show resolved Hide resolved
solc/CommandLineInterface.cpp Outdated Show resolved Hide resolved
@ekpyron ekpyron mentioned this pull request Aug 5, 2020
3 tasks
@Marenz
Copy link
Contributor

Marenz commented Aug 13, 2020

Updated

Changelog.md Outdated
@@ -6,6 +6,7 @@ Language Features:
Compiler Features:
* Standard JSON Interface: Do not run EVM bytecode code generation, if only Yul IR or EWasm output is requested.
* Yul: Report error when using non-string literals for ``datasize()``, ``dataoffset()``, ``linkersymbol()``, ``loadimmutable()``, ``setimmutable()``.
* New feature to stop compilation after parsing stage.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also option the command line option for doing that?

Copy link
Contributor

Choose a reason for hiding this comment

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

you mean "mention"?

libsolidity/interface/CompilerStack.cpp Show resolved Hide resolved
@@ -216,11 +217,11 @@ class CompilerStack: boost::noncopyable

/// Parses and analyzes all source units that were added
/// @returns false on error.
bool parseAndAnalyze();
bool parseAndAnalyze(State _stopAfter = State::CompilationSuccessful);
Copy link
Member

Choose a reason for hiding this comment

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

Referring to my above comment, I think this function should not have that parameter, and callers that needs to stop after parse should then use compile(stopAfterStage) or just call parse() directly?

test/stopAfterParseTests.sh Outdated Show resolved Hide resolved
test/stopAfterParseTests.sh Outdated Show resolved Hide resolved
test/stopAfterParseTests.sh Outdated Show resolved Hide resolved
@Marenz Marenz force-pushed the stopafter branch 3 times, most recently from 68add90 to 9c241ac Compare August 17, 2020 14:00
@Marenz Marenz added the takeover Can be taken over by someone other than label giver label Aug 17, 2020
@Marenz Marenz removed the takeover Can be taken over by someone other than label giver label Aug 24, 2020
@chriseth chriseth force-pushed the stopafter branch 2 times, most recently from 6b488ed to bb3f942 Compare September 2, 2020 10:39
"baseContracts": [],
"contractDependencies": [],
"contractKind": "contract",
"fullyImplemented": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's totally fine that some members are missing, but I'm worried that wrong information might be very misleading.

@chriseth
Copy link
Contributor Author

chriseth commented Sep 2, 2020

The fuzzer somehow interprets a standard-json input as plain solidity.

@chriseth
Copy link
Contributor Author

chriseth commented Sep 3, 2020

I think it really makes sense to use std::optional or SetOnce for all annotations. This will also help in other cases.

@leonardoalt
Copy link
Member

Conflicts

@Marenz Marenz force-pushed the stopafter branch 7 times, most recently from 3c373bd to 96f2605 Compare September 29, 2020 12:08
@Marenz
Copy link
Contributor

Marenz commented Sep 29, 2020

Ready now.

Changelog.md Outdated Show resolved Hide resolved
@Marenz
Copy link
Contributor

Marenz commented Sep 29, 2020

Update: Moved changelog entry to correct version

@chriseth
Copy link
Contributor Author

Needs another rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to output the ast even before import resolution
6 participants