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

WIP: Syntax for creating ImmutableArrays #12859

Draft
wants to merge 48 commits into
base: main
Choose a base branch
from

Conversation

charlesroddie
Copy link
Contributor

Allow creating an ImmutableArray using [: :].
https://github.com/fsharp/fslang-design/blob/main/RFCs/FS-1094-block.md

Module functions done in #11750

@charlesroddie
Copy link
Contributor Author

tests/FSharp.Compiler.Service.Tests/FSharp.CompilerService.SurfaceArea.netstandard.actual was added by a test: should this be gitignored?

@charlesroddie
Copy link
Contributor Author

charlesroddie commented Mar 21, 2022

Main thing needed I think it to implement mkCallSeqToArray: need to understand v_seq_to_array_info in TcGlobals. Edit - possibly done

@vzarytovskii
Copy link
Member

tests/FSharp.Compiler.Service.Tests/FSharp.CompilerService.SurfaceArea.netstandard.actual was added by a test: should this be gitignored?

It can be ignored, I guess, its purpose is to generate new surface area when public APIs have changed. Normally, you just move it to the normal surface area file replacing it.

@charlesroddie
Copy link
Contributor Author

Test failures:

  • members-basics-hw-mutrec, members-basics-hw, type check neg04. Patterns like | [:? R] -> None give parse error FS0010: Unexpected symbol ']' in pattern. Expected ':]' or other token.:? when the tests probably expect them to give some other error.

@charlesroddie
Copy link
Contributor Author

I'd like to compile and run this to see if this PR works:

let x = [: "Hello world" :]
Console.WriteLine(x.Slice(0,0).[0].ToString())

Or alternatively write a test to this effect.

Is there a best way to do this? An existing place where there are tests which compile and run something?

@vzarytovskii

@baronfel
Copy link
Member

When I want to test changes, I often just do dotnet run in the fsi project directory to get dumped into an FSI that understands my new syntax. Maybe that's all you're looking for?

@charlesroddie
Copy link
Contributor Author

charlesroddie commented May 5, 2023

Thanks to those helping out in the amplifying F# session!
At the end:
[: for i = 0 to 100 do yield 42 :] works (computation expression).
[: 42 :] doesn't work, failing at the stage of generating IL. The general problem is that arrays are special, requiring special IL, and lists are also given special treatment in the compiler, so there was nothing standard to copy. The places where it doesn't work are indicated in comments in PR code. Currently Expr.Op (TOp.Block, [argTy], argsR, m) is generated but not handled, but we were looking into pasting in code from LowerComputedListOrArraySeqExpr.

@nojaf
Copy link
Contributor

nojaf commented May 8, 2023

Hi @charlesroddie, I just took another look at this and I think we can try and prevent the transformation from SynExpr.ArrayOrListComputed to SynExpr.ArrayOrList.

If SimpleSemicolonSequence is matched, there will be a conversion, which I think we want to prevent from happening for Immutable arrays.

match comp with
| SimpleSemicolonSequence cenv acceptDeprecatedIfThenExpression elems ->
match comp with
| SimpleSemicolonSequence cenv false _ -> ()
| _ when validateExpressionWithIfRequiresParenthesis -> errorR(Deprecated(FSComp.SR.tcExpressionWithIfRequiresParenthesis(), m))
| _ -> ()
let replacementExpr =
match cType with
| CollectionType.Array ->
// This are to improve parsing/processing speed for parser tables by converting to an array blob ASAP
let nelems = elems.Length
if nelems > 0 && List.forall (function SynExpr.Const (SynConst.UInt16 _, _) -> true | _ -> false) elems
then SynExpr.Const (SynConst.UInt16s (Array.ofList (List.map (function SynExpr.Const (SynConst.UInt16 x, _) -> x | _ -> failwith "unreachable") elems)), m)
elif nelems > 0 && List.forall (function SynExpr.Const (SynConst.Byte _, _) -> true | _ -> false) elems
then SynExpr.Const (SynConst.Bytes (Array.ofList (List.map (function SynExpr.Const (SynConst.Byte x, _) -> x | _ -> failwith "unreachable") elems), SynByteStringKind.Regular, m), m)
else SynExpr.ArrayOrList (cType, elems, m)
| CollectionType.ImmutableArray -> // NOTE: if the compiler moves internally from array to immarray then the optimization above should be moved here
SynExpr.ArrayOrList (cType, elems, m)
| CollectionType.List ->
if elems.Length > 500 then
error(Error(FSComp.SR.tcListLiteralMaxSize(), m))
SynExpr.ArrayOrList (cType, elems, m)
TcExprUndelayed cenv overallTy env tpenv replacementExpr
| _ ->

Something like:

    match cType, comp with
    | (CollectionType.Array | CollectionType.List), SimpleSemicolonSequence cenv acceptDeprecatedIfThenExpression elems -> 

If the | _ -> path is followed for [: 2; 3; 4 :] everything will play out.

That only leaves us with the empty immutable array case, [: :], which should be invoking System.Collections.Immutable.ImmutableArray<T> Create<T> () when TOp.Block is hit in IlxGen.fs.

@nojaf
Copy link
Contributor

nojaf commented May 8, 2023

That only leaves us

That is actually only true for expressions. Patterns and nice print should still be addressed as well.

@edgarfgp
Copy link
Contributor

edgarfgp commented May 9, 2023

@charlesroddie I think we also need to put this behind a LangPreview flag

@vzarytovskii
Copy link
Member

@charlesroddie I think we also need to put this behind a LangPreview flag

Yeah, that definitely needs to go under flag, but it should be relatively easy addition, we can first wrap the codegen and solve open questions.

Not still entirely sure about recursive tast approach VS just emitting into codegen directly. Cc @dsyme

@charlesroddie
Copy link
Contributor Author

It would be ideal to emit codegen to convert [: 2; 3; 4 :] into ImmutableArray.Create(2, 3, 4) rather than go down the CE path which would create a builder and then Add 2 and 3 and 4 and then ToImmutable. The builder would allocate more memory and would allocate more and more memory whenever it runs out of space, and one of the main advantages of ImmutableArray vs FSharpList is to be compact in memory.

@vzarytovskii
Copy link
Member

It would be ideal to emit codegen to convert [: 2; 3; 4 :] into ImmutableArray.Create(2, 3, 4)

This should be the case for const arrays, yes.

For comprehensions, we should follow what we do for lists.

@edgarfgp
Copy link
Contributor

Would be good to know if there is an interest on completing/including this in compiler and library. If not what is the alternative?
maybe an external library to support this ?

I know Avalonia.FuncUI and Fabulous will benefit a lot from this.

cc @vzarytovskii

@vzarytovskii
Copy link
Member

Would be good to know if there is an interest on completing/including this in compiler and library. If not what is the alternative? maybe an external library to support this ?

I know Avalonia.FuncUI and Fabulous will benefit a lot from this.

cc @vzarytovskii

Probably not in this form, but rather type-directed/user-defined collections as a more generic approach.

@JaggerJo
Copy link

Would be good to know if there is an interest on completing/including this in compiler and library. If not what is the alternative? maybe an external library to support this ?
I know Avalonia.FuncUI and Fabulous will benefit a lot from this.
cc @vzarytovskii

Probably not in this form, but rather type-directed/user-defined collections as a more generic approach.

I'm in favour of a generic approach 👍

Think C# 12's collection expressions are quite handy in that regard.

@edgarfgp
Copy link
Contributor

Would be good to know if there is an interest on completing/including this in compiler and library. If not what is the alternative? maybe an external library to support this ?
I know Avalonia.FuncUI and Fabulous will benefit a lot from this.
cc @vzarytovskii

Probably not in this form, but rather type-directed/user-defined collections as a more generic approach.

Would the team have availability to tackle this one or is up external contributors to work on this ?

@vzarytovskii
Copy link
Member

Would be good to know if there is an interest on completing/including this in compiler and library. If not what is the alternative? maybe an external library to support this ?
I know Avalonia.FuncUI and Fabulous will benefit a lot from this.
cc @vzarytovskii

Probably not in this form, but rather type-directed/user-defined collections as a more generic approach.

Would the team have availability to tackle this one or is up external contributors to work on this ?

Not for F# 9 for sure for the team. It will need to go through suggestion (might be there already), design, RFC, review.

@edgarfgp
Copy link
Contributor

Not for F# 9 for sure for the team. It will need to go through suggestion (might be there already), design, RFC, review.

Maybe this fsharp/fslang-suggestions#1086 ?

@vzarytovskii
Copy link
Member

Not for F# 9 for sure for the team. It will need to go through suggestion (might be there already), design, RFC, review.

Maybe this fsharp/fslang-suggestions#1086 ?

Not only that one, user-defined collections should also be considered (I think they're in the immutable arrays discussion somewhere in comments).

@vzarytovskii
Copy link
Member

In other words - suggestion would be something along the lines of

"Allow collections to be type-directed, allow use of [...] syntax for them, and (optionally) allow user defined collections to follow the same rules.".

First two sound straightforward, whereas last one is a bit more controversial (as well as not fully defined, I think).

@vzarytovskii
Copy link
Member

vzarytovskii commented Dec 22, 2023

This is the one for the last point: fsharp/fslang-suggestions#625
Also, a great discussion about it: fsharp/fslang-design#528 (comment)

As much as I hate "duplicate" suggestions, I think a new one should be made superseding
fsharp/fslang-suggestions#1086
fsharp/fslang-suggestions#625
since both of those are quite old, a bunch of things have changed in C# and CLR (inline arrays, C# support for directed conversions, Spans, stack allocated type-directed array, etc).

and if accepted in more or less generic form, it will cover #15437 as well.

Personally, my problem is that if we introduce another non-directed syntax for immutable arrays, it might be confusing people (i.e. having all of the [], [||], [::]).

I do also see issues with type direction which folks were concerned about in the corresponding suggestion, but I trust we can find middle ground for that.

@edgarfgp
Copy link
Contributor

Thanks @vzarytovskii

@charlesroddie
Copy link
Contributor Author

charlesroddie commented Dec 22, 2023

Would be good to know if there is an interest on completing/including this in compiler and library. If not what is the alternative? maybe an external library to support this ?

Getting this PR ready shouldn't take long. I can fix conflicts. It currently works for [: yield x; yield! y :] expressions but not [: x; z :] expressions since as a beginner I couldn't work out how to invoke ImmutableArray.Create(x, z) from the compiler. Should take another AmplifyingF# session to finish this.

Probably not in this form, but rather type-directed/user-defined collections as a more generic approach.

Type-directed [] without a dedicated syntax would scupper the whole ImmutableArrays in F# concept. People think that type-directed is some sort of magic that would allow them to write for ImmutableArrays (or arrays) [ x; y ] instead of [: x; y :] when in reality it would involve writing

let l: ImmutableArray<int * string> = [ 0, "0"; 1, "1" ]

instead of

let l = [: (0, "0"); (1, "1") :]

The former syntax then no one would use since it is far too verbose, and inferior to what you can use at the moment ImmutableArray.Create((0, "0"), (1, "1")) (which is the annoyance that this PR is trying to solve).

Personally, my problem is that if we introduce another non-directed syntax for immutable arrays, it might be confusing people (i.e. having all of the [], [||], [::]).

Perhaps you could ask people if they would be confused by that? Is anyone confused by [] and [||] currently? I don't see this as something that would typically confuse people and I work with beginners a lot. Type-directed on the other hand is extremely difficult to explain: you would have to say "sometimes this syntax means this and sometimes it means that", and overall the resolution would be to give up on explaining and understanding and just use it and respond to compiler warnings when they come up (as typical users would do with type-directed string vs FormatProvider).

@charlesroddie
Copy link
Contributor Author

charlesroddie commented Dec 22, 2023

The official status of language discussion is:
The RFC has [: :].
A discussion thread inside the RFC discussion suggests type-directed [] instead. Another discussion thread there suggests making [] and [||] user-definable

Both were proposed tentatively by @dsyme, and neither suggestion is a rejection of explicit syntax. For example we could introduce [: :] for immarray, [. .] for list, and leave [] as defaulting to list but free for user-redefinition.

@vzarytovskii
Copy link
Member

Type-directed [] without a dedicated syntax would scupper the whole ImmutableArrays in F# concept. People think that type-directed is some sort of magic that would allow them to write for ImmutableArrays (or arrays) [ x; y ] instead of [: x; y :] when in reality it would involve writing

let l: ImmutableArray<int * string> = [ 0, "0"; 1, "1" ]

instead of

let l = [: (0, "0"); (1, "1") :]

That is not what type-direction means in this particular case. It's all about:

let someFunction (x: ImmutableArray<int>) = ...

// Right now, you should explicitly call it/construct it, whereas with working type-direction, the following will be possible

someFunction [1; 2; 3]

It's not a repolacement for existing syntax for arrays, lists, how you do sets, etc, it's just to provide ability to use unified syntax where type is known.

Also, with the type-direction (putting user-defined [] aside for a moment), it will be possible to have library functions, such as (types are mere examples here):

let inline imarray (xs: ImmutableArray<'T>) = xs
let inline array (xs: Array<'T>) = xs
let inline list (xs: List<'T>) = xs
let inline queue (xs: Queue<'T>) = XS

// and have the ability to declare collections as

let ary = immaray [1;2;3;4]
let l = list [1;2;3;4]

Personally, my problem is that if we introduce another non-directed syntax for immutable arrays, it might be confusing people (i.e. having all of the [], [||], [::]).

Perhaps you could ask people if they would be confused by that? Is anyone confused by [] and [||] currently?

Yeah, every single time I show F# to new people/people from different languages, the question is "why can't compiler decide what's better for me by default? I just want to use []`

All I'm saying is that I think, that this problem should be tackled at the more fundamental level, and not just "let's slap new syntax here, and all is good". Again, not saying we don't want to have "new syntax" for immutable array. It's maybe just better to have it as a compiler feature, and not compiler intrinsic. That's why I suggest we revisit the above suggestions, discussions to see what's is the thing we really want.

@vzarytovskii
Copy link
Member

vzarytovskii commented Dec 22, 2023

neither suggestion is a rejection of explicit syntax

But neither has consensus either unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

9 participants