Expose "parsingAndImporting" setting to user #12199

Marenz · 2021-10-27T11:16:40Z

chriseth · 2021-10-27T14:30:49Z

I don't understand - why do we need a new "stop after" setting? I thought the idea would be to just fill the absolute path during parsing and before actually trying to load the file?

Marenz · 2021-10-27T14:55:00Z

The ticket specified

Either perform import resolution earlier so that absolutePath is already in the AST when stopAfter: parsing is used or introduce a new value for stopAfter (parsingAndImportResolution?) that still stops before analysis but after import resolution.

and I figured we have that internal step already so just exposing it would be the easiest solution.. and I figured it makes sense to expose more steps as necessary..

chriseth · 2021-10-27T15:11:17Z

I think that, unless it is complicated to do, we should expose more useful data earlier.

chriseth · 2021-10-28T16:15:07Z

libsolidity/interface/CompilerStack.cpp

@@ -1102,12 +1116,8 @@ StringMap CompilerStack::loadMissingSources(SourceUnit const& _ast, std::string
 			{
 				solAssert(!import->path().empty(), "Import path cannot be empty.");


Can you check for import->annotation().absolutePath instead now?

It turns out: No. The path can actually be empty after we resolved it. What was previously "." becomes empty. But we still have the assert pre-resolve stage as it was originally...

libsolidity/interface/CompilerStack.cpp

chriseth · 2021-10-28T16:18:28Z

Changelog.md

@@ -8,6 +8,7 @@ Compiler Features:
 * Commandline Interface: Accept nested brackets in step sequences passed to ``--yul-optimizations``.
 * Commandline Interface: Add ``--debug-info`` option for selecting how much extra debug information should be included in the produced EVM assembly and Yul code.
 * Commandline Interface: Use different colors when printing errors, warnings and infos.
+ * General: Absolute paths of imports are now set in the ``parsing`` stage.


We could use "now" in every entry, so I think it is better to not use it at all.

Oh and as category, maybe "JSON AST"? Because that is what is relevant to the user reading the changelog.

The option is available using the command line as well though?

Sure, but the json ast is a generic thing that is available to all ways to access the compiler.

leonardoalt · 2021-11-03T10:53:14Z

Tests failing

chriseth · 2021-11-08T13:35:05Z

libsolidity/interface/CompilerStack.cpp

+			for (auto const& import: ASTNode::filteredNodes<ImportDirective>(source.ast->nodes()))
+				{
+					solAssert(!import->path().empty(), "Import path cannot be empty.");
+
+					// The current value of `path` is the absolute path as seen from this source file.
+					// We first have to apply remappings before we can store the actual absolute path
+					// as seen globally.
+					import->annotation().absolutePath =
+						applyRemapping(
+							util::absolutePath(
+								import->path(),
+								path
+							),
+							path
+						);
+				}


Suggested change

for (auto const& import: ASTNode::filteredNodes<ImportDirective>(source.ast->nodes()))

{

solAssert(!import->path().empty(), "Import path cannot be empty.");

// The current value of `path` is the absolute path as seen from this source file.

// We first have to apply remappings before we can store the actual absolute path

// as seen globally.

import->annotation().absolutePath =

applyRemapping(

util::absolutePath(

import->path(),

path

),

path

);

}

for (auto const& import: ASTNode::filteredNodes<ImportDirective>(source.ast->nodes()))

{

solAssert(!import->path().empty(), "Import path cannot be empty.");

// The current value of `path` is the absolute path as seen from this source file.

// We first have to apply remappings before we can store the actual absolute path

// as seen globally.

import->annotation().absolutePath = applyRemapping(util::absolutePath(

import->path(),

path

), path);

}

chriseth · 2021-11-08T15:14:44Z

test/cmdlineTests/standard_parsing_import_absolute_paths/output.json

+        {
+            "ast":
+            {
+                "absolutePath": "/project/../C.sol",


Interesting!

Ah ok, it was named like that in the input json.

chriseth · 2021-11-08T15:15:41Z

test/cmdlineTests/standard_parsing_import_absolute_paths/output.json

+        {
+            "ast":
+            {
+                "absolutePath": "/project/../C.sol",


Ah ok, it was named like that in the input json.

Marenz force-pushed the expose-parsingAndImporting branch 2 times, most recently from a255e20 to 70a7c70 Compare October 27, 2021 12:30

Marenz force-pushed the expose-parsingAndImporting branch 2 times, most recently from 1cda14c to 0a6852b Compare October 28, 2021 14:10

chriseth reviewed Oct 28, 2021

View reviewed changes

libsolidity/interface/CompilerStack.cpp Outdated Show resolved Hide resolved

chriseth reviewed Oct 28, 2021

View reviewed changes

libsolidity/interface/CompilerStack.cpp Outdated Show resolved Hide resolved

chriseth reviewed Oct 28, 2021

View reviewed changes

Marenz force-pushed the expose-parsingAndImporting branch from 0a6852b to 3a5142a Compare October 28, 2021 16:53

Marenz force-pushed the expose-parsingAndImporting branch from 3a5142a to 6a2ea7b Compare November 8, 2021 13:08

chriseth reviewed Nov 8, 2021

View reviewed changes

Marenz force-pushed the expose-parsingAndImporting branch 2 times, most recently from 139b5cd to f860891 Compare November 8, 2021 15:10

chriseth force-pushed the expose-parsingAndImporting branch from f860891 to 1ed2977 Compare November 8, 2021 15:14

chriseth reviewed Nov 8, 2021

View reviewed changes

chriseth previously approved these changes Nov 8, 2021

View reviewed changes

chriseth enabled auto-merge November 8, 2021 15:16

Evaluate absolute paths for imports in 'parsing' stage

fc224f7

chriseth dismissed their stale review via fc224f7 November 8, 2021 15:34

chriseth force-pushed the expose-parsingAndImporting branch from 1ed2977 to fc224f7 Compare November 8, 2021 15:34

chriseth approved these changes Nov 8, 2021

View reviewed changes

chriseth disabled auto-merge November 8, 2021 16:06

chriseth merged commit 1633e36 into develop Nov 8, 2021

chriseth deleted the expose-parsingAndImporting branch November 8, 2021 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose "parsingAndImporting" setting to user #12199

Expose "parsingAndImporting" setting to user #12199

Marenz commented Oct 27, 2021

chriseth commented Oct 27, 2021

Marenz commented Oct 27, 2021

chriseth commented Oct 27, 2021

chriseth Oct 28, 2021

Marenz Nov 8, 2021

chriseth Oct 28, 2021

chriseth Oct 28, 2021

Marenz Oct 28, 2021

chriseth Oct 29, 2021

leonardoalt commented Nov 3, 2021

chriseth Nov 8, 2021

chriseth Nov 8, 2021

chriseth Nov 8, 2021

chriseth Nov 8, 2021

		@@ -1102,12 +1116,8 @@ StringMap CompilerStack::loadMissingSources(SourceUnit const& _ast, std::string
		{
		solAssert(!import->path().empty(), "Import path cannot be empty.");

Expose "parsingAndImporting" setting to user #12199

Expose "parsingAndImporting" setting to user #12199

Conversation

Marenz commented Oct 27, 2021

chriseth commented Oct 27, 2021

Marenz commented Oct 27, 2021

chriseth commented Oct 27, 2021

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

leonardoalt commented Nov 3, 2021

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