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.

Sign up for GitHub

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

Jump to bottom

Autoscope syntax #9372

Merged
merged 6 commits into from
Mar 12, 2024
Merged

Autoscope syntax #9372

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3151cf3
Rename suspended-default-arguments syntax
kazcw Mar 11, 2024
fbddabc
Implement autoscope operator
kazcw Mar 11, 2024
aa63ee6
Clean up.
kazcw Mar 11, 2024
bedad3b
: is an ordinary operator
kazcw Mar 12, 2024
897a980
Disallow autoscope in patterns
kazcw Mar 12, 2024
7e82455
Disallow autoscope of lowercase identifiers
kazcw Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 12 additions & 17 deletions engine/runtime-parser/src/main/java/org/enso/compiler/core/TreeToIr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ private Expression translateCall(Tree ast, boolean isMethod) throws SyntaxExcept
var tree = ast;
for (;;) {
switch (tree) {
case Tree.App app when app.getArg() instanceof Tree.AutoScope -> {
case Tree.App app when app.getArg() instanceof Tree.SuspendedDefaultArguments -> {
kazcw marked this conversation as resolved.
Show resolved Hide resolved
hasDefaultsSuspended = true;
tree = app.getFunc();
}
Expand Down Expand Up @@ -670,16 +670,6 @@ && isDotOperator(oprApp.getOpr().getRight())
var loc = getIdentifiedLocation(oprApp.getLhs());
args.add(new CallArgument.Specified(Option.empty(), self, loc, meta(), diag()));
}
} else if (tree instanceof Tree.OprApp oprApp
&& isDotDotOperator(oprApp.getOpr().getRight())
&& oprApp.getRhs() instanceof Tree.Ident ident) {
var methodName = buildName(ident);
func = new Name.MethodReference(
Option.empty(),
methodName,
methodName.location(),
meta(), diag()
);
} else if (args.isEmpty()) {
return null;
} else {
Expand Down Expand Up @@ -1064,12 +1054,21 @@ loc, meta(), diag()
case Tree.App app -> {
var fn = translateExpression(app.getFunc(), isMethod);
var loc = getIdentifiedLocation(app);
if (app.getArg() instanceof Tree.AutoScope) {
if (app.getArg() instanceof Tree.SuspendedDefaultArguments) {
yield new Application.Prefix(fn, nil(), true, loc, meta(), diag());
} else {
yield fn.setLocation(loc);
}
}
case Tree.AutoscopedIdentifier autoscopedIdentifier -> {
kazcw marked this conversation as resolved.
Show resolved Hide resolved
var methodName = buildName(autoscopedIdentifier.getIdent());
yield new Name.MethodReference(
Option.empty(),
methodName,
methodName.location(),
meta(), diag()
);
}
case Tree.Invalid __ -> translateSyntaxError(tree, Syntax.UnexpectedExpression$.MODULE$);
default -> translateSyntaxError(tree, new Syntax.UnsupportedSyntax("translateExpression"));
};
Expand Down Expand Up @@ -1103,7 +1102,7 @@ Tree applySkip(Tree tree) {
case Tree.BodyBlock ignored -> null;
case Tree.Number ignored -> null;
case Tree.Wildcard ignored -> null;
case Tree.AutoScope ignored -> null;
case Tree.SuspendedDefaultArguments ignored -> null;
case Tree.ForeignFunction ignored -> null;
case Tree.Import ignored -> null;
case Tree.Export ignored -> null;
Expand Down Expand Up @@ -1880,10 +1879,6 @@ private static boolean isDotOperator(Token.Operator op) {
return op != null && ".".equals(op.codeRepr());
}

private static boolean isDotDotOperator(Token.Operator op) {
kazcw marked this conversation as resolved.
Show resolved Hide resolved
return op != null && "..".equals(op.codeRepr());
}

private static Tree maybeManyParensed(Tree t) {
for (;;) {
switch (t) {
Expand Down
4 changes: 2 additions & 2 deletions engine/runtime-parser/src/test/java/org/enso/compiler/core/EnsoParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,15 +934,15 @@ public void testOperatorSectionRight() throws Exception {
}

@Test
public void testAutoScope() throws Exception {
public void testSuspendedDefaultArguments() throws Exception {
parseTest("""
fn that_meta =
c_2 = that_meta.constructor ...
""");
}

@Test
public void testAutoScope2() throws Exception {
public void testSuspendedDefaultArguments2() throws Exception {
parseTest("""
fn1 = fn ...
fn2 = fn 1 ...
Expand Down
2 changes: 1 addition & 1 deletion lib/rust/parser/debug/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ where T: serde::Serialize + Reflect {
vec![Digits::reflect(), NumberBase::reflect(), Operator::reflect(), TextSection::reflect()];
let stringish_tokens = stringish_tokens.into_iter().map(|t| rust_to_meta[&t.id]);
let skip_tokens = vec![
AutoScope::reflect(),
SuspendedDefaultArguments::reflect(),
CloseSymbol::reflect(),
Newline::reflect(),
OpenSymbol::reflect(),
Expand Down
21 changes: 19 additions & 2 deletions lib/rust/parser/debug/tests/parse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,6 +865,23 @@ fn method_app_in_minus_unary() {
(UnaryOprApp "-" (OprApp (Ident Number) (Ok ".") (Ident positive_infinity))));
}

#[test]
fn autoscope_operator() {
test!("x : ..True", (TypeSignature (Ident x) ":" (AutoscopedIdentifier ".." True)));
test!("x = ..True", (Assignment (Ident x) "=" (AutoscopedIdentifier ".." True)));
test!("x = f ..True",
(Assignment (Ident x) "=" (App (Ident f) (AutoscopedIdentifier ".." True))));
expect_invalid_node("x = ..4");
expect_invalid_node("x = ..Foo.Bar");
JaroslavTulach marked this conversation as resolved.
Show resolved Hide resolved
expect_invalid_node("x = f .. True");
expect_invalid_node("x = f(.. ..)");
expect_invalid_node("x = f(.. *)");
expect_invalid_node("x = f(.. True)");
expect_multiple_operator_error("x = ..");
expect_multiple_operator_error("x = .. True");
expect_multiple_operator_error("x : .. True");
}


// === Import/Export ===

Expand Down Expand Up @@ -1269,15 +1286,15 @@ fn case_by_type() {
}

#[test]
fn pattern_match_auto_scope() {
fn pattern_match_suspended_default_arguments() {
#[rustfmt::skip]
let code = [
"case self of",
" Vector_2d ... -> x",
];
#[rustfmt::skip]
let expected = block![
(CaseOf (Ident self) #(((() (App (Ident Vector_2d) (AutoScope)) "->" (Ident x)))))];
(CaseOf (Ident self) #(((() (App (Ident Vector_2d) (SuspendedDefaultArguments)) "->" (Ident x)))))];
test(&code.join("\n"), expected);
}

Expand Down
7 changes: 6 additions & 1 deletion lib/rust/parser/src/lexer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ impl<'s> Lexer<'s> {
}
// Composed of operator characters, but not an operator node.
"..." => {
let token = token.with_variant(token::Variant::auto_scope());
let token = token.with_variant(token::Variant::suspended_default_arguments());
self.submit_token(token);
}
// Decimal vs. method-application must be distinguished before parsing because they
Expand Down Expand Up @@ -714,6 +714,11 @@ fn analyze_operator(token: &str) -> token::OperatorProperties {
.with_unary_prefix_mode(token::Precedence::max())
.as_compile_time_operation()
.as_suspension(),
".." =>
kazcw marked this conversation as resolved.
Show resolved Hide resolved
return operator
.with_unary_prefix_mode(token::Precedence::min_valid())
.as_compile_time_operation()
.as_autoscope(),
"@" =>
return operator
.with_unary_prefix_mode(token::Precedence::max())
Expand Down
13 changes: 12 additions & 1 deletion lib/rust/parser/src/syntax/token.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ macro_rules! with_token_definition { ($f:ident ($($args:tt)*)) => { $f! { $($arg
Wildcard {
pub lift_level: u32
},
AutoScope,
SuspendedDefaultArguments,
Ident {
pub is_free: bool,
pub lift_level: u32,
Expand Down Expand Up @@ -340,6 +340,7 @@ pub struct OperatorProperties {
is_arrow: bool,
is_sequence: bool,
is_suspension: bool,
is_autoscope: bool,
JaroslavTulach marked this conversation as resolved.
Show resolved Hide resolved
is_annotation: bool,
is_dot: bool,
is_special: bool,
Expand Down Expand Up @@ -427,6 +428,11 @@ impl OperatorProperties {
Self { is_suspension: true, ..self }
}

/// Return a copy of this operator, modified to be flagged as the autoscope operator.
pub fn as_autoscope(self) -> Self {
Self { is_autoscope: true, ..self }
}

/// Return a copy of this operator, modified to be flagged as the dot operator.
pub fn as_dot(self) -> Self {
Self { is_dot: true, ..self }
Expand Down Expand Up @@ -492,6 +498,11 @@ impl OperatorProperties {
self.is_suspension
}

/// Return whether this operator is the autoscope operator.
pub fn is_autoscope(&self) -> bool {
self.is_autoscope
}

/// Return whether this operator is the annotation operator.
pub fn is_annotation(&self) -> bool {
self.is_annotation
Expand Down
21 changes: 17 additions & 4 deletions lib/rust/parser/src/syntax/tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,9 @@ macro_rules! with_ast_definition { ($f:ident ($($args:tt)*)) => { $f! { $($args)
#[reflect(as = "i32")]
pub de_bruijn_index: Option<u32>,
},
/// The auto-scoping marker, `...`.
AutoScope {
pub token: token::AutoScope<'s>,
/// The suspended-default-arguments marker, `...`.
SuspendedDefaultArguments {
pub token: token::SuspendedDefaultArguments<'s>,
},
TextLiteral {
pub open: Option<token::TextStart<'s>>,
Expand Down Expand Up @@ -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 •
edited
Loading

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

pub opr: token::Operator<'s>,
pub ident: token::Ident<'s>,
},
/// Defines the point where operator sections should be expanded to lambdas. Let's consider
/// the expression `map (.sum 1)`. It should be desugared to `map (x -> x.sum 1)`, not to
/// `map ((x -> x.sum) 1)`. The expression `.sum` will be parsed as operator section
Expand Down Expand Up @@ -951,6 +956,14 @@ pub fn apply_unary_operator<'s>(opr: token::Operator<'s>, rhs: Option<Tree<'s>>)
false => Tree::annotated(opr, token, None, vec![], None),
};
}
if opr.properties.is_autoscope() && let Some(rhs) = rhs {
return if let box Variant::Ident(Ident { mut token }) = rhs.variant {
token.left_offset = rhs.span.left_offset;
Tree::autoscoped_identifier(opr, token)
} else {
Tree::unary_opr_app(opr, Some(rhs)).with_error("The auto-scope operator (..) may only be applied to an identifier.")
}
}
if !opr.properties.can_form_section() && rhs.is_none() {
let error = format!("Operator `{opr:?}` must be applied to an operand.");
let invalid = Tree::unary_opr_app(opr, rhs);
Expand Down Expand Up @@ -990,7 +1003,7 @@ pub fn to_ast(token: Token) -> Tree {
Tree::text_literal(default(), default(), vec![newline], default(), default())
}
token::Variant::Wildcard(wildcard) => Tree::wildcard(token.with_variant(wildcard), default()),
token::Variant::AutoScope(t) => Tree::auto_scope(token.with_variant(t)),
token::Variant::SuspendedDefaultArguments(t) => Tree::suspended_default_arguments(token.with_variant(t)),
token::Variant::OpenSymbol(s) =>
Tree::group(Some(token.with_variant(s)), default(), default()).with_error("Unmatched delimiter"),
token::Variant::CloseSymbol(s) =>
Expand Down
Loading