feat: generalized imports supporting pattern matching and selective field import #3076

ggreif · 2022-01-27T15:12:36Z

Resolves #2354, the bulk of it.

We can now import subsets of modules:

import { cons; nil = empty } = "lib/ListM";

let s = cons(1, empty());

Yay!

These will be done separately:

importing of type fields explicitly,
dealing with explicit imports in the languageServer (languageServer support for explicit symbol imports #3078),
importing from actors: test/run-drun/explicit-actor-import.mo.

mostly failing now

github-actions · 2022-01-27T16:00:26Z

Comparing from 7437bc1 to af6941a:
In terms of gas, no changes are observed in 3 tests.
In terms of size, no changes are observed in 3 tests.

src/mo_def/compUnit.ml

and push it into the module

doesn't pass IR checker yet

crusso · 2022-01-27T19:13:11Z

doc/modules/language-guide/examples/grammar.txt

@@ -299,6 +299,7 @@

 <imp> ::= 
    'import' <id>? '='? <text>
+    'import' '{' <list(<pat_field>, ';')> '}' '=' <text>


why not just <pat> = text?

Agreed the syntax here ought to simply be <pat> (or <pat_nullary>).

Doesn't that allow tuple patterns and all the other things (wildcard and literals)?
What would those even mean?
Or should we just delegate those to the type-checker (will error out) and accept _ as a NOP import?

You could insist the pattern is irrefutable, I think, as with shared function args.

Cool, that's what I do. I have also added tests for it.

@crusso the pattern matches arising from imports have this form

(LetD (ObjP (cons (VarP cons)) (nil (VarP nil))) (VarE file$lib/ListM.mo))

but we have little control about the RHS (VarE file$lib/ListM.mo). What we could do is, when in check_ir, detect ids like file$* and run check (Ir_utils.is_irrefutable p) on the LHS pattern. Is this what you mean?

Well, we know a bit about the RHS, after all. It gets built in desugar.ml/transform_import and we can anticipate names. Would it be possible to have an IrrefutableLetD and insert that in transform_import?

@crusso what about 4c94e00?

Okay now I reuse the coverage check from typing.ml but make it an error when its is happening in an error. PTAL! 2d9b797

JFTR, <pat> gives

menhir mo_frontend/parser.{ml,mli} (exit 1) (cd _build/default && /nix/store/ih98div9nccad7gjvksxwzhyl0jag1m7-ocaml4.12.0-menhir-20211012/bin/menhir --table --inspection -v --strict mo_frontend/parser.mly --base mo_frontend/parser) Error: one state has shift/reduce conflicts. Warning: one shift/reduce conflict was arbitrarily resolved.

doc/modules/language-guide/examples/grammar.txt

test/fail/not-static.mo

test/fail/self-import.mo

src/mo_def/compUnit.ml

src/mo_frontend/parser.mly

now we get the best desugaring: ``` scheme (LetD (ObjP (cons (VarP cons)) (nil (VarP nil))) (VarE file$lib/ListM.mo)) ```

src/docs/extract.ml

src/mo_frontend/typing.ml

doc/modules/language-guide/pages/language-manual.adoc

Co-authored-by: Claudio Russo <[email protected]>

crusso · 2022-01-28T20:56:22Z

src/mo_def/compUnit.ml

+  let import_decs =
+    List.map (fun { it = (pat, fp, ri); at; note} ->
+        { it = LetD (pat,
+                     { it = ImportE (fp, ri);
+                       at;
+                       note = { note_typ = note; note_eff = Type.Triv} });
+          at;
+          note = { note_typ = note; note_eff = Type.Triv } }
+      ) imports


Use the identation style on the left - I think that's what Andreas was advocating... (at least my understanding of it).

This is what emacs gave me, will change.

crusso · 2022-01-28T21:07:52Z

doc/modules/language-guide/examples/grammar.txt

@@ -298,7 +298,7 @@
    <obj_body>

 <imp> ::= 
-    'import' <id>? '='? <text>
+    'import' <pat_nullary> '='? <text>


Is there any reason this isn't just <pat> as for let?

We don't want (?(#foo)) here, do we? <pat> just doesn't have any sensible use-case in this place.

Yes, but I don't think we should enforce typing properties using the grammar.
Also, it keeps the grammar closer to the informal language reference.
Plus, you might want a type annotation on the pattern without having to parenthesize it.

Can I do this tomorrow in a new PR? I am totally burnt out, sorry.

Tomorrow is saturday. Monday!

It doesn't work: #3076 (comment)

crusso

For safety, I would add a quick test/run-drun test to see this works on imports to compiled actors - it should be fine, but the code for compilation units is more complicated than it should be and something could be going wrong.

ggreif · 2022-01-28T21:42:44Z

For safety, I would add a quick test/run-drun test to see this works on imports to compiled actors - it should be fine, but the code for compilation units is more complicated than it should be and something could be going wrong.

I get errors: af6941a, but that is fine, I think.

crusso · 2022-01-28T21:50:28Z

For safety, I would add a quick test/run-drun test to see this works on imports to compiled actors - it should be fine, but the code for compilation units is more complicated than it should be and something could be going wrong.

I get errors: af6941a, but that is fine, I think.

Right, but that won't exercise the codegen path.

Maybe add another or different test (Like your List client in test/run above, but enclode in actor {...}), that actually compiles and runs.

BTW, do we allow actor patterns and module patterns, if you specify the keyword (actor or module)? Indeed, it's a bit odd that we can use a record pattern to deconstruct something that is declared as a module, not object (shouldn't the full pattern syntax be <obj_sort> {field_pat *}, but never mind...)

crusso · 2022-01-28T22:51:06Z

(wrote you a drun test in #3081 - to merge into this branch)

Additional test for #3076 (testing wasm execution on IC)

@kritzcreek

This PR adds straightforward "go to definition" support for Motoko's pattern-style import syntax. For example, `import { v|als } "mo:base/Array"` and `v|als(x)` (where `|` represents the cursor) will correctly navigate to the corresponding `Array.vals` declaration. As with existing language server functionality, these changes do not yet account for variable shadowing. However, the updated `parse_module_header` function already parses import patterns such as (`import {alias = field} "..."`) based on the proposed generalized import syntax (#3076). Please let me know if additional explanation would be helpful for any of these changes. I also included four test cases to cover the newly expected behavior of `definition_handler` and `parse_module_header`. Tested in VSCode on WSL using the official Motoko extension. Fixes #3078 Supersedes #3263 CC @kritzcreek, @crusso, @matthewhammer

first shot at #2354

b46e72a

ggreif changed the title ~~Explicit symbol imports~~ Explicit symbol imports Jan 27, 2022

ggreif added 2 commits January 27, 2022 16:29

add test

e6e9915

mostly failing now

improve a bit

67500bf

ggreif marked this pull request as draft January 27, 2022 15:53

ggreif added 2 commits January 27, 2022 17:29

unit grooming

2d40582

cleaner

1556274

ggreif commented Jan 27, 2022

View reviewed changes

src/mo_def/compUnit.ml Outdated Show resolved Hide resolved

ggreif added 2 commits January 27, 2022 17:53

resolve the type error at the access site

3b120ee

and push it into the module

resolve the module vs. field of module ambiguity

bcf3fcb

doesn't pass IR checker yet

crusso reviewed Jan 27, 2022

View reviewed changes

doc/modules/language-guide/examples/grammar.txt Outdated Show resolved Hide resolved

ggreif added 3 commits January 27, 2022 22:01

thread the module note through

59efd68

clean up

957f912

get the module VarE's type right

a1ee29c

ggreif force-pushed the gabor/deconstruct-import branch from bf4606a to a1ee29c Compare January 27, 2022 21:33

ggreif added 4 commits January 27, 2022 22:36

cleanup

91c3708

make it 'import' <pat_nullary> '='? <text>

8d7fb3c

pointless

8fe6866

also pointless

ddcef0f

ggreif commented Jan 27, 2022

View reviewed changes

test/fail/not-static.mo Show resolved Hide resolved

ggreif commented Jan 27, 2022

View reviewed changes

test/fail/self-import.mo Show resolved Hide resolved

ggreif commented Jan 27, 2022

View reviewed changes

src/mo_def/compUnit.ml Outdated Show resolved Hide resolved

ggreif commented Jan 27, 2022

View reviewed changes

src/mo_frontend/parser.mly Outdated Show resolved Hide resolved

ggreif added 4 commits January 27, 2022 23:28

Update src/mo_frontend/parser.mly

f581947

capture the entire pattern

0eff5af

now we get the best desugaring: ``` scheme (LetD (ObjP (cons (VarP cons)) (nil (VarP nil))) (VarE file$lib/ListM.mo)) ```

cleanups

999d654

make comp_unit_of_prog linear-time

c32448f

ggreif commented Jan 27, 2022

View reviewed changes

src/docs/extract.ml Outdated Show resolved Hide resolved

ggreif added 2 commits January 28, 2022 21:17

add example

c98c798

ooops

a3ac786

crusso reviewed Jan 28, 2022

View reviewed changes

src/mo_frontend/typing.ml Show resolved Hide resolved

crusso changed the title ~~Explicit symbol imports~~ feat: generalized imports supporting pattern matching and selective field import Jan 28, 2022

crusso self-requested a review January 28, 2022 20:38

crusso reviewed Jan 28, 2022

View reviewed changes

doc/modules/language-guide/pages/language-manual.adoc Show resolved Hide resolved

crusso reviewed Jan 28, 2022

View reviewed changes

doc/modules/language-guide/pages/language-manual.adoc Outdated Show resolved Hide resolved

ggreif and others added 2 commits January 28, 2022 21:50

Update doc/modules/language-guide/pages/language-manual.adoc

2ae3cbb

Co-authored-by: Claudio Russo <[email protected]>

Update language-manual.adoc

3e8226c

crusso reviewed Jan 28, 2022

View reviewed changes

reindent

b5ed7d0

crusso reviewed Jan 28, 2022

View reviewed changes

tweak again

1ddcf93

crusso approved these changes Jan 28, 2022

View reviewed changes

ggreif added 3 commits January 28, 2022 22:28

Merge branch 'master' into gabor/deconstruct-import

95a93a8

move an item

ca01c14

demonstrate that actor methods cannot be imported explicitly

af6941a

ggreif added the automerge-squash When ready, merge (using squash) label Jan 28, 2022

crusso mentioned this pull request Jan 28, 2022

test: drun test for pattern-based imports #3081

Merged

mergify bot merged commit 09dcba8 into master Jan 28, 2022

mergify bot removed the automerge-squash When ready, merge (using squash) label Jan 28, 2022

mergify bot pushed a commit that referenced this pull request Jan 28, 2022

test: drun test for pattern-based imports (#3081)

b03bb7a

Additional test for #3076 (testing wasm execution on IC)

ggreif deleted the gabor/deconstruct-import branch January 29, 2022 00:06

q-uint mentioned this pull request Feb 23, 2022

Selective Field Import #3121

Closed

ggreif mentioned this pull request Mar 8, 2022

Support selective field import for types. #3145

Open

rvanasa mentioned this pull request Jun 2, 2022

Add basic language server support for explicit symbol imports #3282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: generalized imports supporting pattern matching and selective field import #3076

feat: generalized imports supporting pattern matching and selective field import #3076

ggreif commented Jan 27, 2022 •

edited

Loading

github-actions bot commented Jan 27, 2022 •

edited

Loading

crusso Jan 27, 2022 •

edited

Loading

rossberg Jan 27, 2022

ggreif Jan 27, 2022 •

edited

Loading

ggreif Jan 27, 2022

crusso Jan 28, 2022

ggreif Jan 28, 2022

ggreif Jan 28, 2022 •

edited

Loading

ggreif Jan 28, 2022

ggreif Jan 28, 2022 •

edited

Loading

ggreif Jan 31, 2022

crusso Jan 28, 2022

ggreif Jan 28, 2022

ggreif Jan 28, 2022

crusso Jan 28, 2022

ggreif Jan 28, 2022 •

edited

Loading

crusso Jan 28, 2022

ggreif Jan 28, 2022

crusso Jan 28, 2022

crusso Jan 28, 2022

ggreif Jan 31, 2022

crusso left a comment •

edited

Loading

ggreif commented Jan 28, 2022

crusso commented Jan 28, 2022 •

edited

Loading

crusso commented Jan 28, 2022

feat: generalized imports supporting pattern matching and selective field import #3076

feat: generalized imports supporting pattern matching and selective field import #3076

Conversation

ggreif commented Jan 27, 2022 • edited Loading

github-actions bot commented Jan 27, 2022 • edited Loading

crusso Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreif Jan 27, 2022 • edited Loading

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

ggreif Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreif Jan 28, 2022 • edited Loading

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

ggreif Jan 28, 2022 • edited Loading

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

crusso left a comment • edited Loading

Choose a reason for hiding this comment

ggreif commented Jan 28, 2022

crusso commented Jan 28, 2022 • edited Loading

crusso commented Jan 28, 2022

ggreif commented Jan 27, 2022 •

edited

Loading

github-actions bot commented Jan 27, 2022 •

edited

Loading

crusso Jan 27, 2022 •

edited

Loading

ggreif Jan 27, 2022 •

edited

Loading

ggreif Jan 28, 2022 •

edited

Loading

ggreif Jan 28, 2022 •

edited

Loading

ggreif Jan 28, 2022 •

edited

Loading

crusso left a comment •

edited

Loading

crusso commented Jan 28, 2022 •

edited

Loading