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

feat: add repro for issue #4323 and cater for a conformity annotation #4324

Merged
merged 21 commits into from
Jan 10, 2024

Conversation

ggreif
Copy link
Contributor

@ggreif ggreif commented Dec 14, 2023

reproduce feature request #4323 and implement it

This won't restrict the resulting type (as pointed out by @rossberg), but merely verifies the interface. public and type annotations on bindings inside the block are there for shaping the former (just like in class definitions).

TODO:

Copy link

github-actions bot commented Dec 14, 2023

Comparing from bd6bfa3 to fab8dd4:
In terms of gas, 1 tests regressed and the mean change is +0.2%.
In terms of size, no changes are observed in 5 tests.

doc/md/examples/grammar.txt Outdated Show resolved Hide resolved
@ggreif ggreif requested a review from crusso December 15, 2023 13:09
@ggreif ggreif marked this pull request as ready for review December 15, 2023 13:09
@crusso
Copy link
Contributor

crusso commented Dec 15, 2023

Cool. Have you got a follow on PR? This one makes the grammar a bit weird (pulling the equal sign into the annotation grammar), so I'd rather review the effect of both in case there's a cleaner refactoring.

{ let t = Option.join t in
let sort = Type.(match s.it with
| Actor -> "actor" | Module -> "module" | Object -> "object"
| _ -> assert false) in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This covers Memory, but that cannot happen via obj_sort, so this is dead.

@crusso crusso mentioned this pull request Dec 15, 2023
Use the same procedure as with `ClassD`, viz. adding an optional type to `ObjBlockE` and checking for it after inferring the body's type.
leave "strange" productions like
``` Motoko
module M : module {} {}
```
where `}` and `{` collide, but we can rectify that
by coming up with a formatter rule to insert `=`
in-between
@ggreif ggreif changed the title chore: add repro for issue #4323 feat: add repro for issue #4323 and cater for conformity annotation Jan 8, 2024
@ggreif ggreif changed the title feat: add repro for issue #4323 and cater for conformity annotation feat: add repro for issue #4323 and cater for a conformity annotation Jan 8, 2024
@@ -150,7 +150,7 @@ x * y + x

A declaration list is not itself (immediately) an *expression*, so we cannot (immediately) declare another variable with its final value (`3`).

**Block expressions.** We can form a *block expression* from this list of declarations by enclosing it with matching *curly braces*. Blocks are only allowed as sub-expressions of control flow expressions like `if`, `loop`, `case`, etc. In all other places, we use `do { … }` to represent block expression, to distinguish blocks from object literals. For example, `do {}` is the empty block of type `()`, while `{}` is an empty record of record type `{}`.
**Block expressions.** We can form a *block expression* from this list of declarations by enclosing it with matching *curly braces*. Blocks are only allowed as sub-expressions of control flow expressions like `if`, `loop`, `case`, etc. In all other places, we use `do { … }` to represent block expression, to distinguish blocks from object literals. For example, `do {}` is the empty block of type `()`, while `{}` is an empty record of record type `{}`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was an invisible character in do { … }, I deleted it, maybe I shouldn't have!

@@ -292,7 +292,7 @@
<dec_nonvar> ::=
'let' <pat> '=' <exp>
'type' <id> ('<' <list(<typ_bind>, ',')> '>')? '=' <typ>
<obj_sort> <id>? '='? <obj_body>
<obj_sort> <id>? (':' <typ>)? '='? <obj_body>
Copy link
Contributor Author

@ggreif ggreif Jan 8, 2024

Choose a reason for hiding this comment

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

In the chapter of object declarations (grep for "where <obj_body> is of the form =? { <dec-field>;* }"), <obj_body> is said to contain the =?, but it is not consistent all over this document. This needs to be fixed-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was a "hairline space" 0x200b. The =? problem was resolved in d1007d1.

doc/md/language-manual.md Outdated Show resolved Hide resolved
doc/md/language-manual.md Outdated Show resolved Hide resolved
@@ -2434,7 +2445,7 @@ and infer_dec env dec : T.typ =
display_typ_expand t'
display_typ_expand t''
| Some typ, T.Actor ->
local_error env dec.at "M0135" "actor class has non-async return type"
local_error env dec.at "M0135"(*FIXME: repeated?*) "actor class has non-async return type"
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to fix this, though it is a previous problem.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

LGTM

ggreif and others added 3 commits January 9, 2024 16:40
Co-authored-by: Claudio Russo <claudio@dfinity.org>
Co-authored-by: Claudio Russo <claudio@dfinity.org>
@ggreif ggreif added the automerge-squash When ready, merge (using squash) label Jan 10, 2024
@ggreif
Copy link
Contributor Author

ggreif commented Jan 10, 2024

@Mergifyio refresh

Copy link
Contributor

mergify bot commented Jan 10, 2024

refresh

✅ Pull request refreshed

@ggreif
Copy link
Contributor Author

ggreif commented Jan 10, 2024

Closes #4327.

@mergify mergify bot merged commit 1235c57 into master Jan 10, 2024
17 checks passed
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jan 10, 2024
@ggreif ggreif added typing Involves the type system feature New feature or request labels Jan 10, 2024
@ggreif ggreif deleted the gabor/4323 branch January 24, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request typing Involves the type system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants