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

: operator is left-associative #11671

Merged
merged 4 commits into from
Nov 27, 2024
Merged

: operator is left-associative #11671

merged 4 commits into from
Nov 27, 2024

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Nov 26, 2024

Pull Request Description

Adjust operator parsing to allow chained conversions, like 3.14 : Integer : Text.

Change the precedence and associativity of the : operator, when used as a binary operator in an expression:

  • It is now left-associative
  • It now has lower precedence than -> (previously they were equal)

Important Notes

One previously-reasonable syntax has changed interpretation: x->x:Type is no longer a valid way to write a casting function, and would likely result in a type error. There was 1 instance of this syntax in our .enso sources.

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,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

Allows chained conversions, like `3.14 : Integer : Text`.

I have verified with `parse_all_enso_files.sh` that this doesn't change the
interpretation of any of the libs or .enso tests; no one is likely relying on
the current behavior.
@kazcw kazcw self-assigned this Nov 26, 2024
@kazcw kazcw marked this pull request as ready for review November 26, 2024 16:27
@kazcw kazcw marked this pull request as draft November 26, 2024 16:39
@kazcw
Copy link
Contributor Author

kazcw commented Nov 26, 2024

This actually conflicts with a syntax that is covered by unit tests, but not currently used--using case expressions to match by type:

case foo of
    f:A->B -> x

I will see if I can reconcile this syntax with chained conversions by adjusting precedence also.

@kazcw kazcw marked this pull request as ready for review November 26, 2024 18:03
@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Nov 27, 2024
@mergify mergify bot merged commit 7eca04a into develop Nov 27, 2024
40 checks passed
@mergify mergify bot deleted the wip/kw/chained-conversions branch November 27, 2024 04:45
}

/// Return the precedence for any operator.
/// Return the lowest precedence for any operator.
pub fn min_valid() -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

min_valid is relatively cryptic name.

'*' | '/' | '%' => 16,
'^' => 17,
_ => 18,
'!' => token::Precedence::Not,
Copy link
Member

Choose a reason for hiding this comment

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

The use of enum makes the code more readable.

pub fn min_valid() -> Self {
Precedence { value: 1 }
debug_assert_eq!(Precedence::Assignment as u8, Precedence::Min as u8 + 1);
Copy link
Member

Choose a reason for hiding this comment

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

An assert that two enum values are next to each other!? Can it be false? Shouldn't it be apparent or a check to verify in a test and not code?

lhs_section_termination: Some(SectionTermination::Reify),
is_right_associative: true,
Copy link
Member

Choose a reason for hiding this comment

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

I have it! This is the line that changes the behavior! The rests are just refactorings.

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 and the precedence change--: and = both had precedence 2, and now they have different levels in the named level order.

@kazcw kazcw restored the wip/kw/chained-conversions branch November 27, 2024 14:28
mergify bot pushed a commit that referenced this pull request Dec 3, 2024
Some simplification enabled by the refactor in #11671. Inspired by review from @JaroslavTulach.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants