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

Introduce --no-import-callback CLI option #14610

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

nikola-matic
Copy link
Collaborator

@nikola-matic nikola-matic commented Oct 13, 2023

Fixes: #14484

  • --no-import-calback
  • Tests
  • Docs

@nikola-matic nikola-matic requested a review from cameel October 13, 2023 09:32
@nikola-matic nikola-matic self-assigned this Oct 13, 2023
@nikola-matic nikola-matic force-pushed the introduce-no-import-callback-cli-option branch 2 times, most recently from 4e8528a to 8441a4b Compare October 13, 2023 09:37
Comment on lines 6 to 7
"formattedMessage": "Cannot import url (\"standard_no_import_callback_urls/A.sol\"): File reader callback is disabled.",
"message": "Cannot import url (\"standard_no_import_callback_urls/A.sol\"): File reader callback is disabled.",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This I find somewhat suspicious, and kinda stumbled upon it by accident. Reading sources via URLs in the standard compiler requires having the file reader callback set, which means that --no-import-callback essentially disabled this feature (i.e. clashes with --allowed-paths. The question now is - firstly, is this OK, and if yes, should I think disallow usage of --no-import-callback and --allowed-paths together?

Copy link
Member

Choose a reason for hiding this comment

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

I see you're slowly discovering all the intricacies of the callback and VFS the hard way, like I did back when I dug into this topic last time :) I think it would be better for you not to retread that path, and just read through the docs I wrote back then: Import Path Resolution. It's a dull topic, but you should know it, because there are a lot of quirks related to file loading and imports that start to make sense when you know how it works as a whole.

The urls thing is mentioned in Initial Content of the Virtual Filesystem. The name itself shows that it's more than just a filesystem loader. Historically, the idea would be that imports are not necessarily filesystem paths. They're just some unique identifiers that your callback can interpret and use to load the source from. For example Remix to this day allows you to import files directly from Github - which is achieved via a custom callback. urls is an extension to that - it lets you load a file from an external source but without embedding that knowledge into the import. Your import refers to a generic source unit name and in Standard JSON you provide a list of URLs where the content matching that source unit name can be found. The first one that can be loaded by your callback is used. For example, that way you could have input that will use OZ from your local machine when available and if not, it will try to load it from github. Or you could keep a version embedded in the URL to make sure the file does not change under you, but not have to repeat it every time you import the file in you sources.

which means that --no-import-callback essentially disabled this feature

Yes, that was my intention here. The main use case of --no-import-callback is to have the compiler treat Standard JSON as self-contained, without reaching into the filesystem. And the current behavior fits that use case very well.

clashes with --allowed-paths

I can't see how they would clash. Allowed paths are a feature of the default callback. If you disable the callback, they have no effect.

should I think disallow usage of --no-import-callback and --allowed-paths together

Yeah, it makes sense to disallow using them together, but more due to the fact that you would never need to do that and allowing them both may give a different impression.

For context: there are 3 options that end up being stored in FileReader: --allow-paths, --include-path and --base-path. Of those --allow-paths is the only one relevant to the callback but not to processing files given on the CLI. The latter two are relevant to both - they are needed to translate paths to source unit names, i.e. the normalized names that we use in CompilerStack and everywhere inside the compiler to refer to files.

@nikola-matic nikola-matic force-pushed the introduce-no-import-callback-cli-option branch 2 times, most recently from c2d6457 to b254475 Compare October 13, 2023 12:01
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Here's some comments. I also pushed a few smaller tweaks as a fixup.

Changelog.md Outdated Show resolved Hide resolved
test/solc/CommandLineParser.cpp Outdated Show resolved Hide resolved
libsolidity/interface/UniversalCallback.h Outdated Show resolved Hide resolved
Comment on lines 6 to 7
"formattedMessage": "Cannot import url (\"standard_no_import_callback_urls/A.sol\"): File reader callback is disabled.",
"message": "Cannot import url (\"standard_no_import_callback_urls/A.sol\"): File reader callback is disabled.",
Copy link
Member

Choose a reason for hiding this comment

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

I see you're slowly discovering all the intricacies of the callback and VFS the hard way, like I did back when I dug into this topic last time :) I think it would be better for you not to retread that path, and just read through the docs I wrote back then: Import Path Resolution. It's a dull topic, but you should know it, because there are a lot of quirks related to file loading and imports that start to make sense when you know how it works as a whole.

The urls thing is mentioned in Initial Content of the Virtual Filesystem. The name itself shows that it's more than just a filesystem loader. Historically, the idea would be that imports are not necessarily filesystem paths. They're just some unique identifiers that your callback can interpret and use to load the source from. For example Remix to this day allows you to import files directly from Github - which is achieved via a custom callback. urls is an extension to that - it lets you load a file from an external source but without embedding that knowledge into the import. Your import refers to a generic source unit name and in Standard JSON you provide a list of URLs where the content matching that source unit name can be found. The first one that can be loaded by your callback is used. For example, that way you could have input that will use OZ from your local machine when available and if not, it will try to load it from github. Or you could keep a version embedded in the URL to make sure the file does not change under you, but not have to repeat it every time you import the file in you sources.

which means that --no-import-callback essentially disabled this feature

Yes, that was my intention here. The main use case of --no-import-callback is to have the compiler treat Standard JSON as self-contained, without reaching into the filesystem. And the current behavior fits that use case very well.

clashes with --allowed-paths

I can't see how they would clash. Allowed paths are a feature of the default callback. If you disable the callback, they have no effect.

should I think disallow usage of --no-import-callback and --allowed-paths together

Yeah, it makes sense to disallow using them together, but more due to the fact that you would never need to do that and allowing them both may give a different impression.

For context: there are 3 options that end up being stored in FileReader: --allow-paths, --include-path and --base-path. Of those --allow-paths is the only one relevant to the callback but not to processing files given on the CLI. The latter two are relevant to both - they are needed to translate paths to source unit names, i.e. the normalized names that we use in CompilerStack and everywhere inside the compiler to refer to files.

@cameel cameel changed the title Introduce no import callback cli option Introduce --no-import-callback CLI option Oct 13, 2023
@nikola-matic nikola-matic force-pushed the introduce-no-import-callback-cli-option branch 3 times, most recently from 98daa37 to 5ee3f50 Compare October 16, 2023 10:05
@r0qs r0qs added this to the 0.8.22 milestone Oct 18, 2023
@r0qs r0qs added the feature label Oct 18, 2023
r0qs
r0qs previously approved these changes Oct 18, 2023
@nikola-matic nikola-matic force-pushed the introduce-no-import-callback-cli-option branch from 5ee3f50 to c936c53 Compare October 19, 2023 09:20
@nikola-matic nikola-matic marked this pull request as ready for review October 19, 2023 09:35
@nikola-matic nikola-matic requested review from cameel and r0qs October 19, 2023 09:35
cameel
cameel previously approved these changes Oct 19, 2023
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

I have some minor nitpicks, but other than that everything is in order and I tentatively approve.

Please remember to either squash before merging or fix your commits - the current description is a lie :P

docs/path-resolution.rst Outdated Show resolved Hide resolved
docs/path-resolution.rst Outdated Show resolved Hide resolved
solc/CommandLineParser.cpp Outdated Show resolved Hide resolved
@nikola-matic nikola-matic force-pushed the introduce-no-import-callback-cli-option branch 2 times, most recently from db1d079 to 13dd31e Compare October 19, 2023 11:19
r0qs
r0qs previously approved these changes Oct 19, 2023
@nikola-matic nikola-matic requested a review from cameel October 19, 2023 12:17
@nikola-matic nikola-matic force-pushed the introduce-no-import-callback-cli-option branch from 3443ac5 to f0e93c8 Compare October 19, 2023 15:12
@cameel cameel force-pushed the introduce-no-import-callback-cli-option branch from f0e93c8 to db98eed Compare October 19, 2023 15:17
@cameel cameel removed this from the 0.8.22 milestone Oct 19, 2023
@nikola-matic nikola-matic merged commit 810c446 into develop Oct 19, 2023
1 check passed
@nikola-matic nikola-matic deleted the introduce-no-import-callback-cli-option branch October 19, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a flag to disable the default import callback
3 participants