Skip to content

Require imports to be specified without file extension#1130

Merged
mergify[bot] merged 11 commits intomasterfrom
christoph/import-without-extension
Jan 22, 2020
Merged

Require imports to be specified without file extension#1130
mergify[bot] merged 11 commits intomasterfrom
christoph/import-without-extension

Conversation

@kritzcreek
Copy link
Contributor

Fixes #1127

The one subtle change in here is that previously import M "path" and import M "path/" would resolve to import M "path/lib.mo" whereas now it's required to import M "path/", because otherwise it'll try to resolve path.mo

@kritzcreek kritzcreek requested a review from nomeata January 17, 2020 15:10
@kritzcreek kritzcreek removed the request for review from nomeata January 17, 2020 16:00
@kritzcreek
Copy link
Contributor Author

Still need to fix the language server to properly handle these paths

@kritzcreek kritzcreek changed the title Require imports to be specified without file extension [WIP] Require imports to be specified without file extension Jan 17, 2020
@nomeata
Copy link
Contributor

nomeata commented Jan 17, 2020

The one subtle change in here is that previously import M "path" and import M "path/" would resolve to import M "path/lib.mo" whereas now it's required to import M "path/", because otherwise it'll try to resolve path.mo

Hmm, that defeats a bit the purpose of having a nice-looking import M "mo:pkg".

I think we should do one of these:

  1. Always first look for foo.mo and then lib/foo.mo
  2. Always first look for foo/lib.mo and then foo.mo
  3. Look for both and complain if both are avabilable
    4 drop that feature alltogether

If we expect that imports will have some guessing around them anyways eventually (e.g. multiple search paths, or “use .wasm if there, else try .mo”), then 1, 2, or 3 are naturally. Else maybe 4.

@rossberg, opinions?

@rossberg
Copy link
Contributor

I'm fine with either of the options you list, though for simplicity, would probably pick 1/2 over 3. Regarding which one path takes precedent, what do other systems do?

@nomeata
Copy link
Contributor

nomeata commented Jan 17, 2020

According to https://stackoverflow.com/a/4092446/946226 python (which I kinda took as inspiration here) does 2; directory before file.

@chenyan-dfinity
Copy link
Contributor

If we have both foo.did and foo.mo, which one takes precedence? I think it's .did, just like .mli.
This is another reason that I think --print-deps should output the resolved file path. We don't want re-implement the resolve logic in dfx.

@nomeata
Copy link
Contributor

nomeata commented Jan 18, 2020

That's a good point for returning the full path. Otoh, the point of printing the dependencies is that the build tool (dfx) can possibly generate the missing files. That seems like an unresolvable dilemma.

Maybe we need two modes?

Or print both import URL and (if present) file it resolves to?

@kritzcreek
Copy link
Contributor Author

I've run into problems with breaking the IDE's file resolution here, so I'm working on unifying that with pipeline first and then I'll get back to this PR.

@chenyan-dfinity
Copy link
Contributor

chenyan-dfinity commented Jan 21, 2020

That's a good point for returning the full path. Otoh, the point of printing the dependencies is that the build tool (dfx) can possibly generate the missing files. That seems like an unresolvable dilemma.

Maybe we need two modes?

Or print both import URL and (if present) file it resolves to?

If we can resolve the file, we don't need dfx to generate any files for us. I think we can output the full path if moc can resolve the imports. The unresolved ones become the obligation of dfx. Opened #1148

@kritzcreek kritzcreek force-pushed the christoph/import-without-extension branch from d1906f5 to 2fbd635 Compare January 22, 2020 13:36
@dfinity-ci
Copy link

dfinity-ci commented Jan 22, 2020

In terms of gas, no changes are observed in 1 tests.
In terms of size, no changes are observed in 1 tests.

@kritzcreek
Copy link
Contributor Author

Allright, tests seem to be passing again and the IDE now uses the same file resolution as the pipeline. I've implemented the "file exists" based resolution for directory paths, the easiest way to tell if this is what you'd imagined is probably to look at the resolve_import_test.ml unit tests.

PTAL

@kritzcreek kritzcreek changed the title [WIP] Require imports to be specified without file extension Require imports to be specified without file extension Jan 22, 2020
@kritzcreek kritzcreek added the automerge-squash When ready, merge (using squash) label Jan 22, 2020
@mergify mergify bot merged commit 0cf39d4 into master Jan 22, 2020
@mergify mergify bot deleted the christoph/import-without-extension branch January 22, 2020 14:54
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jan 22, 2020
@ggreif
Copy link
Contributor

ggreif commented Jan 22, 2020

Edit: The error that you provide is nicely self-explanatory. I'll shut up.

What about out-of tree code (at externals?)

Can we provide a warning when .mo is present, but ignore it, and look up the import as-if it didn't have an extension?

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.

[compiler] Don't allow imports with .mo extension

6 participants