Skip to content

Conversation

@kerams
Copy link
Contributor

@kerams kerams commented Nov 22, 2022

Implements fsharp/fslang-suggestions#539.
RFC

+ (unary and binary), - (unary and binary), *, /, %, &&&, |||, <<<, >>> are now allowed for integers in literals as well as attributes. not, &&, || are allowed for bools.

nwWZVw56UN

@kerams kerams requested a review from a team as a code owner November 22, 2022 13:28
@auduchinok
Copy link
Member

@kerams Just to add another test case: please check it also works inside enum type definitions.

@En3Tho
Copy link
Contributor

En3Tho commented Nov 22, 2022

Nice. Now the hardest part - an RFC :D.

@kerams
Copy link
Contributor Author

kerams commented Nov 22, 2022

@auduchinok, no, that definitely won't work without parser changes. I'd prefer not to tackle that in this PR anyway (just because this is useful as it is, and easier to review).

@T-Gro
Copy link
Member

T-Gro commented Nov 22, 2022

LGTM , provided tests are added.

@vzarytovskii
Copy link
Member

vzarytovskii commented Nov 23, 2022

A shower thought - should this go under language flag? It technically changes compiler flow.

@edgarfgp
Copy link
Contributor

@kerams if we can use if else I would expect pattern matching also work. ?

@kerams
Copy link
Contributor Author

kerams commented Nov 23, 2022

@edgarfgp, I've tried

let [<Literal>] switch2 =
    match true with
    | true -> true
    | _ -> false

and it doesn't work.

| Expr.Match (decision = TDSwitch (input = input; cases = [ TCase (DecisionTreeTest.Const (Const.Bool test), TDSuccess ([], targetNum)) ]); targets = [| TTarget (_, t0, _); TTarget (_, t1, _) |]) -> is a condition tailor-made for how logical operations are represented in the TAST, and I don't think that we necessarily want to create some complex expression evaluator here (that's almost TP territory).

T-Gro
T-Gro previously approved these changes Nov 24, 2022
@vzarytovskii
Copy link
Member

vzarytovskii commented Nov 24, 2022

@T-Gro @0101 @KevinRansom this is a non-breaking new language feature, however it changes compilation pipeline . Shall this be put under preview version, or we're fine enabling it for all versions?

@kerams
Copy link
Contributor Author

kerams commented Nov 24, 2022

let [<Literal>] x = 4 ||| 3 is valid today, so you could say this is just an extension of the existing mechanism. Up to you of course.

@vzarytovskii
Copy link
Member

let [<Literal>] x = 4 ||| 3 is valid today, so you could say this is just an extension of the existing mechanism. Up to you of course.

Yeah, it's just changing the codegen, not sure if someone can already rely on it in some tooling/when using quotations, etc.

@kerams
Copy link
Contributor Author

kerams commented Nov 24, 2022

// See also PostTypeCheckSemanticChecks.CheckAttribArgExpr, which must match this precisely

One thing I'm not sure about is this comment.

Edit: Oh, maybe since all the operations get erased and replaced by constants, that following phase does not have to be updated.

@Happypig375
Copy link
Member

We should allow at least all operations from C# for parity.
image

@kerams
Copy link
Contributor Author

kerams commented Nov 27, 2022

Decimals are a no-go at the moment, because you can't even do let [<Literal>] x = 1m. It needs to be special-cased as a static field with an attribute, which is too much faffing for me:).

@Happypig375
Copy link
Member

Happypig375 commented Nov 27, 2022

Should also allow conversion operators between numeric types and LanguagePrimitives.EnumOfValue.

@kerams
Copy link
Contributor Author

kerams commented Nov 27, 2022

Plenty of other things could theoretically be added - comparison and equality operators, even match expressions. However, I don't want to stray away from what has been proposed and approved too much, and make things unnecessarily complicated.

T-Gro
T-Gro previously approved these changes Nov 28, 2022
@T-Gro
Copy link
Member

T-Gro commented Nov 28, 2022

I am for merging this in as-is without preview version restriction and letting others built on top of it with new ideas as needed in separate PRs.

@auduchinok
Copy link
Member

I am for merging this in as-is without preview version restriction

Why? It contradicts the whole point of having the language versions. It's not a bug fix, and new things can now be compiled that a previous language version didn't allow.

@vzarytovskii
Copy link
Member

vzarytovskii commented Nov 29, 2022

I am for merging this in as-is without preview version restriction

Why? It contradicts the whole point of having the language versions. It's not a bug fix, and new things can now be compiled that a previous language version didn't allow.

Yep, was thinking the same, it changes the codegen and compilation pipeline.

We should hide it behind preview version.

@T-Gro
Copy link
Member

T-Gro commented Nov 29, 2022

I am for merging this in as-is without preview version restriction

Why? It contradicts the whole point of having the language versions. It's not a bug fix, and new things can now be compiled that a previous language version didn't allow.

I see it as an extension of a feature that was incomplete.

That being said, it might be bad for developers having different SDK versions between each other (or between them and CI) while using an older F# language version. => you are right.

@kerams
Copy link
Contributor Author

kerams commented Dec 7, 2022

Bump. Does it need Don's blessing?

I have a working draft for expressions in enum definitions, but this PR is a requirement.

@vzarytovskii
Copy link
Member

Bump. Does it need Don's blessing?

I have a working draft for expressions in enum definitions, but this PR is a requirement.

I think we're fine with merging it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants