Separator between member and type annotation interpreted as operator #15923

edgarfgp · 2023-09-04T15:36:53Z

Attempts to fix #4607

Current Behaviour:

I'm learning my way through the Lexing and Parsing phase.

Updated lex.fsl and LexFilter.fs

type IFoo<'T> =
    // Note:  extra space after >
    abstract member Bar<'T> : name: string -> unit

type IFoo<'T> =
    // Note: No extra space after > 
    abstract member Bar<'T>: name: string -> unit

With these changes you will be able to define >: as an operator that was not allowed before

let (>:) a b = a + b

See what CI says
Ask if there is a better way of doing this
Move tests to an appropriate place

edgarfgp · 2023-09-05T08:58:41Z

@vzarytovskii Would it be acceptable to allow now >: as an operator enabling this code to be parsed?

vzarytovskii · 2023-09-05T09:00:38Z

@vzarytovskii Would it be an acceptable change to allow >: to allow this to be parsed?

Not sure, it's been deliberately deprecated, probably for a reason?

edgarfgp · 2023-09-05T09:08:10Z

@vzarytovskii Would it be an acceptable change to allow >: to allow this to be parsed?

Not sure, it's been deliberately deprecated, probably for a reason?

From what I can see, the error might be there to potentially allow >: to be used in the future in F# similar to :>, :?> etc.

Is that in the F# team plans? The error recovery and reporting for this scenario are currently bad

vzarytovskii · 2023-09-05T10:15:25Z

We don't have plans to use it, @dsyme might have some insight on why is it disallowed.

auduchinok

The change to the token processing seems to be good from a glance, but I think that we should not enable new operators unless there's an approved language suggestion.

edgarfgp · 2023-09-06T15:54:34Z

The change to the token processing seems to be good from a glance, but I think that we should not enable new operators unless there's an approved language suggestion.

Thanks for the review. I went through the lex.fsl rules and noticed that we are doing the same for, ie <^ , </, ^> Also, the fact the GREATER and COLON already exist made this a relatively small change(Not having to add a new token GREATER_COLON maybe

This change will make >: no longer a potential reserved operator for the compiler and allow people to define an operator that was not allowed before.

But if you have a better way of dealing with this specific issue, any suggestion is welcome :)

auduchinok · 2023-09-06T16:04:08Z

But if you have a better way of dealing with this specific issue, any suggestion is welcome :)

We can produce an error about this operator during type checking.

dsyme · 2023-09-26T05:02:02Z

@edgarfgp This looks like a nice fix. It's so good to remove these glitches.

Regarding >: - AFAIK there are no plans to use this or other operators containing :

edgarfgp · 2023-10-11T14:16:50Z

By removing the check in for >: at the lex.fsl and update LexFilter.fs, it fixes the issue but at the same times makes possible to define a custom operator :> or even allows to use this in almost every possible scenario that you can imagine which is problematic. So to mitigate this I added a check for the >: during the type checking which cover most of the cases.
But it is impossible to check for cases where it was previously not allowed like:

module >: = ...
module `>:` = ...
type >: = ...

So I would love to get some feedback in the following questions:

How bad would be allow >: to be used as custom operator. ?
We could prevent this by adding a check in parser level to check when :> is used as module, namespace, type names ?

T-Gro · 2023-10-12T07:07:32Z

By removing the check in for >: at the lex.fsl and update LexFilter.fs, it fixes the issue but at the same times makes possible to define a custom operator :> or even allows to use this in almost every possible scenario that you can imagine which is problematic. So to mitigate this I added a check for the >: during the type checking which cover most of the cases. But it is impossible to check for cases where it was previously not allowed like:
module >: = ...
module `>:` = ...
type >: = ...
So I would love to get some feedback in the following questions:

How bad would be allow >: to be used as custom operator. ?

We could prevent this by adding a check in parser level to check when :> is used as module, namespace, type names ?

Since there are no plans for using it by F# itself, I do not see a reason for not allowing this as a custom operator.
(also the code would get simpler if typechecker would not have to actively guard against it...)

psfinaki

Good stuff. IMO one of the things that should just work and is otherwise confusing for newcomers. Thanks! :)

edgarfgp · 2023-10-12T10:09:36Z

(also the code would get simpler if typechecker would not have to actively guard against it...)

Yeah if we allowing this new operator i think we can avoid guarding against it in the typechecker

edgarfgp · 2023-10-12T10:11:52Z

Will update this PR to remove check in the typechecker

…-interpreted-as-operator

T-Gro · 2023-10-13T07:40:16Z

Good, looks cleaner now.

T-Gro · 2023-10-13T12:11:09Z

@edgarfgp :

The allowed custom operators are listed at https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/symbol-and-operator-reference/ .

Which is this .md document in the docs repo:
https://github.com/dotnet/docs/blob/main/docs/fsharp/language-reference/symbol-and-operator-reference/index.md

Could you please add it as well, since it is now an allowed pair? (and was not before)

edgarfgp · 2023-10-16T15:10:58Z

Could you please add it as well, since it is now an allowed pair? (and was not before)

Sure. Will do it this week

brianrourkeboll · 2023-10-17T20:02:38Z

It looks like this new operator group …>…:… has the same precedence and associativity as any other infix operator starting with >, right?

edgarfgp · 2023-10-18T13:57:27Z

It looks like this new operator group …>…:… has the same precedence and associativity as any other infix operator starting with >, right?

Yes that is my understanding. @T-Gro For my own education which one is the correct here ?

…ses (#16079) * Add analyzer & code fix for IDE0047 * Consistency * Consolidate lambda branches * Try-finally tweak * Fix * Fantomas * Explain * More Fantomas * Pass in getTextAtRange for № literals * Fantomas * Structs * Don't need that * Spaces * getTextAtRange → getSourceLineStr * Use `getSourceLineStr` and enable handling sensitive indentation inside parens. * Singleton * ↓ * Better * Streamline * One more * Don't need that here * Simplify lambda traversal * Add comment * Make private * Fix infix op tests * Add some more tests * Be (somewhat) more systematic about precedence, &c. * Represent precedence for more kinds of expression than just symbolic infix operators. * Handle exprs potentially confusable with type applications. * Handle more numeric literal cases. * Handle dangling nested exprs (`match`, `if`, &c.) * Consolidate SynPat logic * Comments * Space after backticks * Parens in interp hole * Submodule * Move extension method to common module * Expose raverseAll internally for clarity * Remove redundant arrow * Move diagnostic creation logic into its own file * We're still calling the logic directly inside of `DocumentDiagnosticAnalyzer` rather than exporting the type as an implementation of the `IFSharpUnnecessaryParenthesesDiagnosticAnalyzer` interface, since that interface does not exist upstream in Roslyn, and it seems like we would currently prefer not to go through the logistics of upstreaming it. * Fantomas * Remove comment * Apply Fantomas to tests * Include paren diagnostics in builder capacity * Update surface area * Remove unnecessary parens in DocumentDiagnosticAnalyzer tests * Add in-memory cache like SimplifyNameDiagnosticAnalyzer's * Update misleading comment * Ish * Might as well include the other example * The prefix op rules are tricky… * +Assorted other bits and bobs * Use existing helper method * More correct dangling expr logic * A few more tests for dangling exprs * Fix incomplete exprs * Missed that * Structness is immaterial here * Fantomas * Remove semi-misleading word * Add example to doc comment * Move ROS extensions to separate file in FCS * Use FS3583 instead of IDE0047 * Add example to comment * Fix find/replace in comments * Do the faster check first * Pretty sure we don't actually need that * Upon further thought, I'm pretty sure we _don't_ need to memoize the checking for dangling right-hand expressions, because we never dive deeper than needed for the check at hand, and the check will be quite shallow unless there are vast amounts of, say, nested matches inside of an if-condition body, but even then, such a check is both necessary and will only happen once, e.g.: ```fsharp let f x = if (match x with | _ -> match x with | _ -> match x with | _ -> match x with | _ -> match x with | _ -> if x then x else x) then x else x ``` If any other if-expression were found, or any nested parenthesized expression, or any construct without an exposed right-hand expression, the dive would end. * Fix (nonfunctional) copy/paste bug * Add tests for unmatched parens * Consolidate affixed infix tests for speed * Fantomas * Address a few more corner cases * Add basic ServiceAnalysis tests * Remove redundant assertions * Move some framework-y code to the framework file * Remove incorrect test * This test purported to test FS1182, but the actual diagnostic that is generated from the example is FS1183. * Don't need * A few more tests * Add a few attribute-related tests * Add test for new operator introduced in #15923 * Vars instead of consts * Add a few clarifying comments * Update tests/FSharp.Compiler.Service.Tests/UnnecessaryParenthesesTests.fs --------- Co-authored-by: Petr <[email protected]>

edgarfgp assigned nojaf and auduchinok and unassigned nojaf and auduchinok Sep 4, 2023

edgarfgp requested review from nojaf and auduchinok September 4, 2023 16:50

edgarfgp marked this pull request as ready for review September 5, 2023 08:52

edgarfgp requested a review from a team as a code owner September 5, 2023 08:52

edgarfgp removed request for nojaf and auduchinok September 6, 2023 12:29

auduchinok suggested changes Sep 6, 2023

View reviewed changes

edgarfgp force-pushed the separator-between-member-and-type-annotation-interpreted-as-operator branch from 99200f1 to 8184817 Compare September 10, 2023 11:57

vzarytovskii added the Branch Lockdown label Sep 26, 2023

psfinaki removed the Branch Lockdown label Oct 4, 2023

edgarfgp added 5 commits October 9, 2023 13:07

Separator between member and type annotation interpreted as operator

8d8e0bb

more testing

a904ddc

more tests

6684f53

move tests

0b0fe3c

WIP more tests

133a5ef

T-Gro approved these changes Oct 12, 2023

View reviewed changes

psfinaki approved these changes Oct 12, 2023

View reviewed changes

Revert check for custom operator during typechecking

a1d5a18

edgarfgp force-pushed the separator-between-member-and-type-annotation-interpreted-as-operator branch from 0b67366 to a1d5a18 Compare October 12, 2023 12:06

Merge branch 'main' into separator-between-member-and-type-annotation…

7df7c1f

…-interpreted-as-operator

T-Gro merged commit 1c0ba9f into dotnet:main Oct 13, 2023

allisonchou mentioned this pull request Oct 17, 2023

[Automated] PRs inserted in VS build main-34217.29 #16129

Closed

brianrourkeboll added a commit to brianrourkeboll/fsharp that referenced this pull request Oct 30, 2023

Add test for new operator introduced in dotnet#15923

deb313f

This was referenced Nov 8, 2023

Parens: handle special parsing of new T… & inherit T… #16239

Merged

Inconsistent handling of arguments for disposable types #15622

Closed

edgarfgp mentioned this pull request Jan 11, 2025

Compiler issue: unexpected error when compiling an abstract generic method #18228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separator between member and type annotation interpreted as operator #15923

Separator between member and type annotation interpreted as operator #15923

edgarfgp commented Sep 4, 2023 •

edited

Loading

edgarfgp commented Sep 5, 2023 •

edited

Loading

vzarytovskii commented Sep 5, 2023

edgarfgp commented Sep 5, 2023 •

edited

Loading

vzarytovskii commented Sep 5, 2023

auduchinok left a comment

edgarfgp commented Sep 6, 2023 •

edited

Loading

auduchinok commented Sep 6, 2023

dsyme commented Sep 26, 2023

edgarfgp commented Oct 11, 2023 •

edited

Loading

T-Gro commented Oct 12, 2023

psfinaki left a comment

edgarfgp commented Oct 12, 2023 •

edited

Loading

edgarfgp commented Oct 12, 2023

T-Gro commented Oct 13, 2023

T-Gro commented Oct 13, 2023

edgarfgp commented Oct 16, 2023

brianrourkeboll commented Oct 17, 2023

edgarfgp commented Oct 18, 2023 •

edited

Loading

Separator between member and type annotation interpreted as operator #15923

Separator between member and type annotation interpreted as operator #15923

Conversation

edgarfgp commented Sep 4, 2023 • edited Loading

edgarfgp commented Sep 5, 2023 • edited Loading

vzarytovskii commented Sep 5, 2023

edgarfgp commented Sep 5, 2023 • edited Loading

vzarytovskii commented Sep 5, 2023

auduchinok left a comment

Choose a reason for hiding this comment

edgarfgp commented Sep 6, 2023 • edited Loading

auduchinok commented Sep 6, 2023

dsyme commented Sep 26, 2023

edgarfgp commented Oct 11, 2023 • edited Loading

T-Gro commented Oct 12, 2023

psfinaki left a comment

Choose a reason for hiding this comment

edgarfgp commented Oct 12, 2023 • edited Loading

edgarfgp commented Oct 12, 2023

T-Gro commented Oct 13, 2023

T-Gro commented Oct 13, 2023

edgarfgp commented Oct 16, 2023

brianrourkeboll commented Oct 17, 2023

edgarfgp commented Oct 18, 2023 • edited Loading

edgarfgp commented Sep 4, 2023 •

edited

Loading

edgarfgp commented Sep 5, 2023 •

edited

Loading

edgarfgp commented Sep 5, 2023 •

edited

Loading

edgarfgp commented Sep 6, 2023 •

edited

Loading

edgarfgp commented Oct 11, 2023 •

edited

Loading

edgarfgp commented Oct 12, 2023 •

edited

Loading

edgarfgp commented Oct 18, 2023 •

edited

Loading