Skip to content

Path resolution docs (current state only) #11263

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

Closed
wants to merge 8 commits into from

Conversation

cameel
Copy link
Member

@cameel cameel commented Apr 15, 2021

Replaces #11105.
Related to #11138, #11036 and #9353.

This is a reworked version of #11105 that only describes the current state, without any proposed modifications. The goal is to get it merged and included in a release before we start changing how this stuff works.

To implement #11105 (review), I have created a separate section called "Path Resolution" in the side menu. The "Layout of a Solidity Source File" now only contains a short summary of the part needed to understand import paths.

The biggest change compared to the original docs is that any mention of absolute imports has been removed and now I only distinguish between "direct" and "relative" imports. I have also moved a lot of the extra details to notes/warnings to separate them from the main flow of the text and simplified them slightly where I could.


import "github.com/ethereum/dapp-bin/library/iterable_mapping.sol" as it_mapping;
Unlike in case of a direct import, the compiler does assume that the import path is actually a path.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this complicating the isuse a little? Whta does it mean to assume it is a path? Isn't the rule just "if it starts with ./ or ../, append it to the path and simplify"?

Copy link
Member Author

Choose a reason for hiding this comment

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

It assumes it's structured like a path, e.g. expects path separators or squashes ../ segments. It's even processed internally using boost::filesystem.

The import path is translated into a source unit name and then the compiler uses the name
to look up the file in its virtual filesystem.
The are two types of imports, each with different rules for this translation:
:ref:`direct imports <direct-imports>` let you specify the full source unit name while in
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not "absolute"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because when I see absolute imports, I think about absolute paths in the real filesystem and I really want to avoid that connotation. Thinking about absolute imports was really giving me the wrong idea about how this works when I was trying to understand this initially. I think we should have a different word to refer to them.

.. note::

Do not confuse relative imports with relative paths.
Both ``util.sol`` and ``./util.sol`` specify relative paths on disk but these paths are treated
Copy link
Contributor

Choose a reason for hiding this comment

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

But this depends on the compiler binary. This section is rather confusing - maybe we should clarify it as early as possible and talk about "filesystem paths relative to the current working directory"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm leaving it here for now but I'll probably rewrite it for the blog post anyway. In #11442 I removed it almost completely and just added a short note.


- All the ``./`` segments are removed.
- Every ``../`` segment backtracks one level up in the hierarchy.
- Multiple slashes are squashed into a single one.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this done to the full path? Maybe we should change that and only modify the initial segment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the part that's in the import statement. The parent path it's combined with is not normalized. This is explained in a note later in the text.

Generally in this rewrite I tried to keep it simple in the initial explanations and only went into detail later in the text or in note/warning boxes.


.. warning::

The root of the virtual filesystem is an empty path, not ``/``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rephrase: An initial / is not required (and also not recommended) for specifying absolute paths in the virtual filesystem.

Copy link
Member Author

Choose a reason for hiding this comment

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

But this would sound like it's optional and the two are equivalent. The empty path does work like the root for us. See for example what happens when you go beyond the root - you get to "" not /.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have rewritten this section completely in #11442.

@cameel cameel force-pushed the path-resolution-docs-current-state branch from 7999aaa to 702e026 Compare May 26, 2021 10:25
@cameel cameel force-pushed the path-resolution-docs-current-state branch from 702e026 to 81e7ae4 Compare May 26, 2021 10:51
@cameel
Copy link
Member Author

cameel commented May 26, 2021

I have just pushed a new version with all the points above addressed. All the changes are in fixups.

I'm going to close it now and split in two parts. One will go into a separate PR with the excessive examples and explanations removed. The other will become a blog post.

@cameel
Copy link
Member Author

cameel commented May 26, 2021

Closing in favor of #11442.

@cameel cameel closed this May 26, 2021
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.

2 participants