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

Autoscope syntax #9372

Merged
merged 6 commits into from
Mar 12, 2024
Merged

Autoscope syntax #9372

merged 6 commits into from
Mar 12, 2024

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Mar 11, 2024

Pull Request Description

Add autoscope syntax (..Ident).

Important Notes

  • Also rename previous Tree.Autoscope to SuspendedDefaultArguments.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 11, 2024
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Way more elegant than my original isDotDotOperator check. Thank you for implementing this so promptly.

lib/rust/parser/src/lexer.rs Show resolved Hide resolved
lib/rust/parser/src/syntax/token.rs Show resolved Hide resolved
@@ -162,6 +162,11 @@ macro_rules! with_ast_definition { ($f:ident ($($args:tt)*)) => { $f! { $($args)
pub opr: token::Operator<'s>,
pub rhs: Option<Tree<'s>>,
},
/// Application of the autoscope operator to an identifier, e.g. `..True`.
AutoscopedIdentifier {
Copy link
Member

Choose a reason for hiding this comment

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

Right now we only support autoscoped constructors. Shouldn't this class be rather called AutoscopedConstructor?

Copy link
Contributor Author

@kazcw kazcw Mar 12, 2024

Choose a reason for hiding this comment

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

That's a good question. I actually called it that originally, but then changed my mind. It felt odd to use the term constructor in the parser, when there is no clear syntactic distinction between a constructor reference and another type of identifier. The parser only knows that the operator has been applied to an identifier token, and I think it's up to the semantic layer to determine whether the ident refers to a constructor (which also has to be one of the constructors of the appropriate type, right?) or to something else entirely. So I guess the question is, should the parser name it based on what it is syntactically, or what it is in its semantically-valid usage? I'm inclined to the former; we'd be forgoing the merits of having the same (semantically-informed) term throughout the stack, but that makes sense to me because it's not the same thing in the parser and in the engine--the engine narrows it based on semantic constraints. Does that make sense to you @JaroslavTulach?

Copy link
Member

Choose a reason for hiding this comment

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

@kazcw Do we still have the syntactic distinction between regular names and type-names/constructors? i.e. lowercase name and Uppercase constructor?

If so, we could raise a syntax error when we encounter the .. operator with a lowercase name - because currently that is not valid syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can narrow it to capital (type-or-constructor) identifiers. I was also just thinking, the parser has 3 contexts: pattern, type, and expression.

  • Clearly this is allowed in expression context.
  • It should be disallowed in type contexts, right? Like x : ..True
  • What about patterns? Would it be a syntax error in the LHS of a case-of arrow?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, currently I think we only allow autoscoping in the expression context.

So it seems sensible to raise a syntax error in the other contexts. @JaroslavTulach is that right?

Copy link
Member

Choose a reason for hiding this comment

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

  • What about patterns?

FYI: https://github.com/orgs/enso-org/discussions/8646#discussioncomment-8604696 - e.g. it is an error to use .. in pattern matching LHS.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, currently I think we only allow autoscoping in the expression context.
So it seems sensible to raise a syntax error in the other contexts. @JaroslavTulach is that right?

The most convincing @wdanilo argument against using ~ was that we couldn't differentiate between type and value level: https://github.com/orgs/enso-org/discussions/8646#discussioncomment-8677685

If we want to differentiate on the parser level between expression context and type context, can we go back to ~ ;-? CCing @jdunkerley

lib/rust/parser/debug/tests/parse.rs Show resolved Hide resolved
@kazcw
Copy link
Contributor Author

kazcw commented Mar 12, 2024

The only file that parses differently after this PR is test/Base_Tests/src/Semantic/Conversion_Spec.enso; the change there is that in the autoscope tests, autoscope is now a syntax.

@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Mar 12, 2024
@mergify mergify bot merged commit a1c0d9a into develop Mar 12, 2024
33 of 35 checks passed
@mergify mergify bot deleted the wip/kw/autoscope-syntax branch March 12, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-compiler -parser -syntax CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants