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

Allow decimal constants #17769

Merged
merged 1 commit into from
Oct 4, 2024
Merged

Conversation

esbenbjerre
Copy link
Contributor

@esbenbjerre esbenbjerre commented Sep 20, 2024

The intention of this PR is to add support for the use of the literal attribute on decimals.

[<Literal>]
let d = 2.5m
  • Allow decimal constant in PostInferenceChecks
  • Add decimal arithmetic in TypedTreeOps
  • Add DecimalConstantAttribute to TcGlobals
  • Add codegen (initonly field with attribute + cctor) to IlxGen
  • Fix broken tests
  • Add test for pattern matching a value against a decimal literal
  • Add test for incomplete pattern matching a value against a decimal literal
  • Add test for usage and evaluation of decimal literal in quotations
  • Add test C# interop

Related fsharp/fslang-suggestions#847

@esbenbjerre esbenbjerre requested a review from a team as a code owner September 20, 2024 10:27
Copy link
Contributor

github-actions bot commented Sep 20, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.200.md

@esbenbjerre
Copy link
Contributor Author

@dotnet-policy-service agree

@T-Gro
Copy link
Member

T-Gro commented Sep 23, 2024

Hi @esbenbjerre , thanks for this contribution.
Here are a few test areas I would expect to be covered as part of this PR:

  • Pattern matching a value against a decimal literal (IL test, compileAndRun test)
  • Incomplete Pattern matching with decimal literals
  • Usage and evaluation of decimal literal in quotations (compileAndRun test)
  • Consumption of the literal in C# (multiproject test for interop), tooling support from C# (referencing a .dll with the decimal constant in a C# project from IDE)

@esbenbjerre
Copy link
Contributor Author

Thanks @T-Gro. I'm still trying to codegen the cctor code. Would you prefer I convert it to draft until it's ready?

@vzarytovskii
Copy link
Member

Yeah, let's convert it to draft so it doesn't pop up on our triage radar

@vzarytovskii vzarytovskii marked this pull request as draft September 23, 2024 12:57
@esbenbjerre
Copy link
Contributor Author

esbenbjerre commented Sep 29, 2024

@T-Gro I have a couple of questions regarding the test requirements.

  • What is expected in terms of IL for pattern matching?
  • For C# interop is it a manual test or automated? And if automated is there anything similar in the repo I could use for inspiration?

@T-Gro
Copy link
Member

T-Gro commented Sep 30, 2024

@T-Gro I have a couple of questions regarding the test requirements.

  • What is expected in terms of IL for pattern matching?
  • For C# interop is it a manual test or automated? And if automated is there anything similar in the repo I could use for inspiration?

For the C# interop, the test framework in "ComponentTests" suite can handle CsharpSource as well.
See e.g. here:

(or if curious, just look at more usages of the "CSharp" function.

For pattern matching a decimal literal, the IL should contain:

  • Init code for the decimal value
  • Loading up of the decimal
  • Calling .Equals() on it against the value being pattern matched.

For inspiration, look at tests which use the "verifyIL" function in the "ComponentTests" suite. I start such tests by writing a minimal F# sample, let the verification fail, and then copy relevant snippets of IL from the test output into the assertion list (that is assuming the IL looks good)

@esbenbjerre esbenbjerre force-pushed the decimal-literal-attribute branch from 3935cda to 4ad236c Compare September 30, 2024 12:00
@esbenbjerre
Copy link
Contributor Author

esbenbjerre commented Oct 1, 2024

@T-Gro I'm getting Invalid assembly name: Name contains invalid characters. when trying to use the CSharp function.
I'm getting it with or without the literal attribute. The code should be valid and does compile in a mixed C#/F# project (sans literal attribute). The same error happens if I change x to an int.

namespace Interop

open Xunit
open FSharp.Test.Compiler

module ``TestDeleteMe`` =

    [<Fact>]
    let ``Instantiate F# decimal literal from C#`` () =
        let FSLib =
            FSharp """
namespace Interop.FS

module DecimalLiteral =
  [<Literal>]
  let x = 7m
        """ |> withName "FSLib"

        let app =
            CSharp """
using System;
using Interop.FS;
public class C {
    public Decimal y = DecimalLiteral.x;
}
        """ |> withReferences [FSLib]

        app
        |> compile
        |> shouldSucceed

results in

[xUnit.net 00:00:03.02]     Interop.TestDeleteMe.Instantiate F# decimal literal from C# [FAIL]
  Failed Interop.TestDeleteMe.Instantiate F# decimal literal from C# [2 s]
  Error Message:
   System.Exception : Operation failed (expected to succeed).
 All errors:
[{ Error = Error 8203
   Range = { StartLine = 0
             StartColumn = 0
             EndLine = 0
             EndColumn = 0 }
   NativeRange = (0,0--0,0)
   Message = "Invalid assembly name: Name contains invalid characters."
   SubCategory = "" }]

  Stack Trace:
     at FSharp.Test.Compiler.Assertions.shouldSucceed(CompilationResult result) in /Users/ebj/Source/fsharp/tests/FSharp.Test.Utilities/Compiler.fs:line 1529
   at Interop.TestDeleteMe.Instantiate F# decimal literal from C#() in /Users/ebj/Source/fsharp/tests/FSharp.Compiler.ComponentTests/Interop/TestDeleteMe.fs:line 29
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

@esbenbjerre
Copy link
Contributor Author

I managed to solve it by adding |> withName "CSharpApp" to the CSharp CompilationUnit.

@esbenbjerre esbenbjerre marked this pull request as ready for review October 2, 2024 19:22
@esbenbjerre esbenbjerre force-pushed the decimal-literal-attribute branch from 99ce53c to 8812aaf Compare October 2, 2024 19:34
@esbenbjerre esbenbjerre changed the title Support literal attribute on decimals Add support for literal attribute to decimals Oct 3, 2024
@edgarfgp
Copy link
Contributor

edgarfgp commented Oct 3, 2024

I think this is worth a small RFC so we can track this later in the language spec.

@vzarytovskii
Copy link
Member

Yes, this will need an rfc

@esbenbjerre esbenbjerre changed the title Add support for literal attribute to decimals Allow decimal constants Oct 4, 2024
@esbenbjerre
Copy link
Contributor Author

Created RFC PR here fsharp/fslang-design#790

@T-Gro T-Gro merged commit b187b80 into dotnet:main Oct 4, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants