Allow modules to be used in type applications #14655

kerams · 2023-01-24T18:27:10Z

Implements fsharp/fslang-suggestions#730.

Includes support for completion filtering.

src/Compiler/pars.fsy

src/Compiler/Service/ServiceInterfaceStubGenerator.fs

edgarfgp · 2023-01-24T18:45:47Z

Amazed by the quality of your contributions. Learning a lot from it.

edgarfgp · 2023-01-24T18:47:49Z

Im curious if this would work for module rec ?

kerams · 2023-01-24T18:58:18Z

Im curious if this would work for module rec ?

I don't understand what you mean. The only place where it matters is if you want to refer to the module you're in:

module rec A.F

let a =   typeof<module A.F> // doesn't work unless A.F is marked as recursive

edgarfgp · 2023-01-24T19:03:03Z

Im curious if this would work for module rec ?

I don't understand what you mean. The only place where it matters is if you want to refer to the module you're in:
module rec A.F

let a =   typeof<module A.F> // doesn't work unless A.F is marked as recursive

I see . You are right , it is not applicable here

src/Compiler/SyntaxTree/SyntaxTree.fs

src/Compiler/pars.fsy

kerams · 2023-01-25T20:11:39Z

src/Compiler/SyntaxTree/LexFilter.fs


        //  module ... ~~~> CtxtModuleHead 
-        | MODULE, _ :: _ -> 
+        | MODULE, _ :: _ when offsideStack |> List.forall (fun x -> match x with CtxtParen (LESS _, _) -> false | _ -> true) -> 


This is the best way to identify that we're within a type application that I could come up with :/

kerams · 2023-01-28T16:06:39Z

How do I guard this with a lang feature?

edgarfgp · 2023-01-28T17:41:25Z

@kerams This is how I do it

Add a new Entry to LanguageFeatures.fs , and LanguageFeatures.fsi
Add a new entry to FSComp.txt and run dotnet build src\Compiler /t:UpdateXlf to generate
now you you can use the new preview flag in code

You can see how I added here https://github.com/dotnet/fsharp/pull/14263/files

kerams · 2023-01-28T17:45:32Z

Yeah, I know. I am specifically concerned about the parsing change. Since lang features are not available in pars.fsy, I could add it to CheckExpressions.fs, but then I have to supply an appropriate error message, rather than relying on the parser to reject such code.

edgarfgp · 2023-01-29T14:31:04Z

Yeah, I know. I am specifically concerned about the parsing change. Since lang features are not available in pars.fsy, I could add it to CheckExpressions.fs, but then I have to supply an appropriate error message, rather than relying on the parser to reject such code.

When review the static abstract interfaces implementation. I found this check we can use similar in this case ?

fsharp/src/Compiler/pars.fsy

Line 1784 in 5061f4d

| STATIC ABSTRACT MEMBER

kerams · 2023-01-30T09:32:58Z

Thank you, Edgar, it works really well. Not sure how I missed it.

nojaf · 2023-01-30T13:52:00Z

src/Compiler/pars.fsy

     { let mDecl = (rhs parseState 1)
       if not $2 then errorR(Error(FSComp.SR.parsTypeNameCannotBeEmpty(), mDecl))
-       SynMemberDefn.Inherit(SynType.LongIdent(SynLongIdent([], [], [])), None, mDecl) }
+       SynMemberDefn.Inherit(SynType.LongIdent(SynLongIdent([], [], []), false), None, mDecl) }


The fact that you need to pass false in so many locations is a bit of a code smell to me.

This is outdatedl

My bad. I'm still quite on the fence about whether SynType.LongIdentModule is the way to go. I think an application would fit the bill better.

The range of SynType.LongIdentModule will currently be incorrect as it will only include the longId range and not start from the module keyword.

The thing is, you can theoretically do f<module M, int> (), so this isn't a new application type. We're just accepting a new kind of type argument - SynType.

The range of SynType.LongIdentModule will currently be incorrect as it will only include the longId range and not start from the module keyword.

Good catch.

I'm not really following, if you were to write f<module2 M, int> () today (link), you get exactly a SynType.App for module2 M.

And regardless of how your model this in SynType, you will find theoretical scenario's where you can plug in module X wherever a SynType is allowed.

But module A.B.C is in and of itself not an application. module is no type (to be stored in typeName), but a disambiguating keyword. Technically it isn't even needed and LongIdent could have been used with no additions (by virtue of the fact that modules are IL classes just like types).

I think we'd just be abusing SynType.App and making it confusing.

module A.B.C is syntactically an application. This is valid jargon for the untyped tree. Just like int list is an application in the untyped tree. Of course, there is no int function that is being applied by a list, but it is a SynType.App nonetheless.

The evaluation of module A.B.C will later, in the typed tree, of course not be an application, but on the untyped level, I'm calling it as I see it.

Ok, I get it now. However, if the parser can already tell module A.B.C and int list are semantically different, shouldn't they be encoded differently? Wouldn't it be cleaner than seeing whether typeName equals module in CheckExpressions.fs?

Well, that is part of the debate, is this really different enough to have its own DU case?
From an untyped perspective, I'd say no. Most SynType are rather undecided yet on what they actually will represent later in the typed tree.

I think #13700 is a bit of an example. We allowed users to write int array2d instead of int[,]. This could have been a new case in the SynType DU, except it just fits in what is already out there. That feature wasn't new from a syntax point of view.

And just because you are using the module keyword, I'm still convinced this is similar.

You have my two cents, ultimately it is not up to me. The maintainers will tell you what they want eventually. If they approve your current code, I would implore you to include the full range of the type, as that will be problematic sooner or later.

Lastly, I do want to mention that this is a solid PR and you have done amazing work here.
I may seem a tad too critical in reviewing this, but that is only because I have a stake in the SyntaxTree. Every change made to it affects me and I will voice my concerns when I deem it necessary. I expect no different from my peers when they review my PRs.
Keep it up!

I would implore you to include the full range of the type

Yeah, I have already done this locally.

I may seem a tad too critical in reviewing this

No, no, I love thorough PR scrutiny and discussing the implementation in order to maintain high standards, especially in a project like the F# compiler. So thanks for taking the time to review this.

nojaf · 2023-01-30T13:55:43Z

Could some tests be added when using module X in strange places gives you the same error as before?

Things like:

type X = module Y
let (a: module Y): module X = ()

should not become valid AST.

kerams · 2023-01-30T16:07:59Z

To my surprise this works

module rec X
let value: module X =  Unchecked.defaultof<module X>

Unfortunately ItemOccurrence does not distinguish between the 2 uses of module X, so I am not sure how to reject the type annotation.

And if we were to support modules in type annotations (I don't see why that would be useful though), then the following doesn't work with the parser and lex filter as is let a (b: module X) = ().

vzarytovskii · 2023-01-30T19:55:45Z

To my surprise this works
module rec X
let value: module X =  Unchecked.defaultof<module X>
Unfortunately ItemOccurrence does not distinguish between the 2 uses of module X, so I am not sure how to reject the type annotation.

And if we were to support modules in type annotations (I don't see why that would be useful though), then the following doesn't work with the parser and lex filter as is let a (b: module X) = ().

Hm, interesting, what does the code above result into? I mean what do we codegen.

I assume we can still parse those, but later on restrict it to the type parameters only?

tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj

T-Gro · 2023-02-01T10:31:36Z

tests/FSharp.Compiler.ComponentTests/Language/ModuleAsTypeTests.fs

+        (Error 39, Line 13, Col 26, Line 13, Col 39, "The namespace or module 'NonexistentNs' is not defined.")
+        (Error 39, Line 14, Col 28, Line 14, Col 39, "The module 'Nonexistent' is not defined.")
+        (Error 39, Line 15, Col 30, Line 15, Col 35, "The module 'value' is not defined.")
+        (Error 39, Line 16, Col 23, Line 16, Col 24, "The module 'R' is not defined.")


Any chance for making this better in the case of 'R' being a type?
i.e. turning the diagnostics into a clippy-like suggestion for trying " typeof without the module keyword" instead.

I will try, it might be tricky. Plus the module keyword might not be present in some context in the future.

tests/FSharp.Compiler.ComponentTests/Language/ModuleAsTypeTests.fs

T-Gro

Left a few minor comments:

Consider more helpful diagnostics when typeof<module .. ends up pointing to a type
Check that typedefof also works (it should)

nojaf · 2023-02-04T13:54:50Z

@kerams would you mind also adding a test in https://github.com/dotnet/fsharp/blob/main/tests/service/SyntaxTreeTests/SynTypeTests.fs for the full range of the SynType?
I think it is appropriate as you changed the grammar rules in pars.fsy.

auduchinok

@kerams This is an interesting change, and it's really cool that it hasn't required many changes to the parsing!

I'm still very worried about any changes made to how type and module are parsed, as these are very special keywords for LexFilter that affect parser recovery a lot.

It could be much safer to get these changes in, if we had a good test suite that would assert that file trees are not changed for various cases where error recovery is kicking in, but there's no such tests as for now. We would need #13484 and do a very careful testing here. Without that it can break parsing in subtle ways when a user is typing source in an editor and having unfinished code.

We really need to check various cases how unfinished expressions and types interact with the new rule and to have it recorded via tests. Consider cases like this, from the top of my head:

module Module1 =
    let x = Unchecked.defaultof<x, // caret here

module Module2 =
    ()

module Module1 =
    let x = f<x,
              module // caret here

    ignore

auduchinok · 2023-02-06T10:48:03Z

src/Compiler/SyntaxTree/LexFilter.fs

-        //  module ... ~~~> CtxtModuleHead 
-        | MODULE, _ :: _ -> 
+        //  module ... ~~~> CtxtModuleHead, unless we're in a type application 
+        | MODULE, _ :: stack when stack |> List.forall (fun x -> match x with CtxtParen (LESS _, _) -> false | _ -> true) -> 


Does this check indentation somewhere, so we don't consider module a part of a type application while expression isn't finished?

module Module1 = // typing an expression here, e.g. `x < 0`, in an editor let f x = x < module Module2 = let y = ()

I.e. LexFilter should produce additional tokens that would end the first module before returning MODULE to the parser.

I haven't tried this yet. My knowledge of this area is very limited, and the above was merely the simplest solution.

auduchinok · 2023-02-06T10:49:52Z

src/Compiler/SyntaxTree/SyntaxTree.fsi

+    /// F# syntax: typeof<module A.B.C>
+    | LongIdentModule of longDotId: SynLongIdent * range: range


Please clarity the doc, so it's clear that typeof< and > are not part of this node.

auduchinok · 2023-02-06T10:55:52Z

src/Compiler/pars.fsy

    { [ SynTupleTypeSegment.Type $1 ] }

 appTypeCon:
+  | moduleKeyword path %prec prec_atomtyp_path 


Can we get MODULE_COMING_SOON or MODULE_IS_HERE here too, or only MODULE is possible? Perhaps, the changes to LexFilter should not produces the first two, as they are the synchronization tokens that don't seem relevant in the SynType context. We could then only use explicit MODULE token here.

Agreed, MODULE would be better, since the module context isn't being pushed here.

auduchinok · 2023-02-06T11:18:29Z

tests/FSharp.Compiler.ComponentTests/Language/ModuleAsTypeTests.fs

+[<Fact>]
+let ``Module can be used as generic type argument in lang preview`` () =
+    Fsx """
+let actual = ResizeArray<module LanguagePrimitives>().GetType().FullName


Is it expected to be correct? What does this code generate?
I think we should only allow it in typeof and typedefof? We could check that during type check and produce an error.

Well, modules are classes, so the IL is valid. While C# doesn't allow static types as type arguments, F# doesn't mind.

Whether anything outside of typeof would be useful is highly questionable, but I'm not sure it's worth banning module type application in general on those grounds. On the other hand, modules in annotations are not allowed... I don't know.

And if we decide typeof is the only valid use, why not just introduce moduleof (though the name is confusing) and avoid mucking around with all these parsing changes, or allow modules in typeof and live with the potential ambiguities. I'd love Don to weigh in here.

Allow modules to be used in type applications

f2fdd72

kerams commented Jan 24, 2023

View reviewed changes

src/Compiler/pars.fsy Show resolved Hide resolved

kerams commented Jan 24, 2023

View reviewed changes

src/Compiler/Service/ServiceInterfaceStubGenerator.fs Outdated Show resolved Hide resolved

auduchinok reviewed Jan 24, 2023

View reviewed changes

src/Compiler/SyntaxTree/SyntaxTree.fs Outdated Show resolved Hide resolved

src/Compiler/pars.fsy Show resolved Hide resolved

kerams added 3 commits January 25, 2023 19:52

Stuff

3dc6e7b

Merge remote-tracking branch 'upstream/main' into typeof-module

d2e8356

Refactor

a8c161b

kerams commented Jan 25, 2023

View reviewed changes

kerams added 4 commits January 28, 2023 09:55

Merge remote-tracking branch 'upstream/main' into typeof-module

5b4002b

Fix surface area

fc33e47

Add tests

3010719

Add completions support

b19e561

kerams marked this pull request as ready for review January 28, 2023 14:29

kerams requested a review from a team as a code owner January 28, 2023 14:29

Guard modules in type application with a language feature

1d152b3

nojaf reviewed Jan 30, 2023

View reviewed changes

Include entire range

4754ad4

T-Gro reviewed Feb 1, 2023

View reviewed changes

tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj Outdated Show resolved Hide resolved

T-Gro reviewed Feb 1, 2023

View reviewed changes

tests/FSharp.Compiler.ComponentTests/Language/ModuleAsTypeTests.fs Outdated Show resolved Hide resolved

T-Gro requested changes Feb 1, 2023

View reviewed changes

kerams added 4 commits February 4, 2023 10:16

Merge

942084e

Fix test

7369049

Add error when used in annotation

e3cd6ab

Fix tests

fcae981

Add range correctness assertions

c4d66c9

auduchinok reviewed Feb 6, 2023

View reviewed changes

kerams closed this Jul 20, 2023

kerams deleted the typeof-module branch July 20, 2023 13:34

		/// F# syntax: typeof<module A.B.C>
		\| LongIdentModule of longDotId: SynLongIdent * range: range

Allow modules to be used in type applications #14655

Allow modules to be used in type applications #14655

Uh oh!

Conversation

kerams commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edgarfgp commented Jan 24, 2023

Uh oh!

edgarfgp commented Jan 24, 2023

Uh oh!

kerams commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edgarfgp commented Jan 24, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kerams commented Jan 28, 2023

Uh oh!

edgarfgp commented Jan 28, 2023

Uh oh!

kerams commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edgarfgp commented Jan 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kerams commented Jan 30, 2023

Uh oh!

nojaf Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kerams Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kerams Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kerams Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojaf commented Jan 30, 2023

Uh oh!

kerams commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vzarytovskii commented Jan 30, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

T-Gro left a comment

Choose a reason for hiding this comment

Uh oh!

nojaf commented Feb 4, 2023

Uh oh!

kerams commented Jan 24, 2023 •

edited

Loading

kerams commented Jan 24, 2023 •

edited

Loading

kerams commented Jan 28, 2023 •

edited

Loading

edgarfgp commented Jan 29, 2023 •

edited

Loading

nojaf Jan 30, 2023 •

edited

Loading

kerams Jan 30, 2023 •

edited

Loading

kerams Jan 30, 2023 •

edited

Loading

kerams Jan 30, 2023 •

edited

Loading

kerams commented Jan 30, 2023 •

edited

Loading

kerams Feb 6, 2023 •

edited

Loading

kerams Feb 6, 2023 •

edited

Loading

kerams Feb 6, 2023 •

edited

Loading