Prevent assignment to literal ILFields

[baronfel]

This PR closes #9785 by adding a check to DelayedSets of ILFields.

If the backing ILField is a Literal, we should not allow the set. This check is pretty easy to do, but I imagine we would also want to do the following as part of this work:

• create a unique error message
• add some tests
• discuss making this an error vs a warning
• other conditions that should not be assignable
• spec/suggestions/RFC requirement?

Error message

Regarding an error message, here is the current message for an incorrect mutable assignment:

This value is not mutable. Consider using the mutable keyword, e.g. 'let mutable <name of value> = expression'

I propose something like:

The literal field '%s' cannot be mutably assigned.

but I'm not sure what the 'call to action' would be in this case for the user. There's not a generally-suggestible action to take for something as straight-up incorrect as this.

Add some tests

There are a number of available constants in the BCL that I could set up a test that should fail for. I just need some pointers as to where the test should go, in light of the recent work on the test framework by @vzarytovskii.

Error vs Warning

I'm of the opinion that this is safely an error, as any current use of this construct fails at runtime and so isn't valid.

Other Non-assignable conditions

Since this is the place, should we also check to ensure that initonly fields are not set here?

Spec/Suggestions/RFC

Do we need one for this change? If so I can draft them.

Before and after

Here's a sample test FSI script I'm using to verify this change does in fact cause errors:

#r "/usr/local/share/dotnet/packs/Microsoft.NETCore.App.Ref/3.1.0/ref/netcoreapp3.1/System.Security.Claims.dll";;
open System.Security.Claims;;
ClaimTypes.AuthenticationInstant <- "foo";;

Under dotnet fsi shipped with 5.0.100-preview.6.20318.15, the following output results:

Microsoft (R) F# Interactive version 10.10.0.0 for F# 4.7
Copyright (c) Microsoft Corporation. All Rights Reserved.

For help type #help;;

> #r "/usr/local/share/dotnet/packs/Microsoft.NETCore.App.Ref/3.1.0/ref/netcoreapp3.1/System.Security.Claims.dll";;

--> Referenced '/usr/local/share/dotnet/packs/Microsoft.NETCore.App.Ref/3.1.0/ref/netcoreapp3.1/System.Security.Claims.dll' (file may be locked by F# Interactive process)

> open System.Security.Claims;;
> ClaimTypes.AuthenticationInstant <- "foo";;
System.MissingFieldException: Field not found: 'System.Security.Claims.ClaimTypes.AuthenticationInstant'.
Stopped due to error

Under dotnet fsi built from this change:

Microsoft (R) F# Interactive version 11.0.0.0 for F# 5.0
Copyright (c) Microsoft Corporation. All Rights Reserved.

For help type #help;;

> #r "/usr/local/share/dotnet/packs/Microsoft.NETCore.App.Ref/3.1.0/ref/netcoreapp3.1/System.Security.Claims.dll";;

--> Referenced '/usr/local/share/dotnet/packs/Microsoft.NETCore.App.Ref/3.1.0/ref/netcoreapp3.1/System.Security.Claims.dll' (file may be locked by F# Interactive process)

> open System.Security.Claims;;
> ClaimTypes.AuthenticationInstant <- "foo";;

  ClaimTypes.AuthenticationInstant <- "foo";;
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/Users/chethusk/oss/fsharp/src/fsharp/fsi/stdin(3,1): error FS3362: Cannot assign '"foo"' to a value marked literal

> ClaimTypes.AuthenticationInstant <- String.init 0 string;;

  ClaimTypes.AuthenticationInstant <- String.init 0 string;;
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/Users/chethusk/oss/fsharp/src/fsharp/fsi/stdin(5,1): error FS3363: Cannot assign a value to another value marked literal

[abelbraaksma]

Interesting and great work, I was looking into the same issue, but got sidetracked :p.

[abelbraaksma]

The literal field '%s' cannot be mutably assigned.

'mutably' sounds a bit weird to me, I'd suggest (considering that in F# we'd call it a literal, in other languages usually a constant, or a constant field):

'%s' is a literal [or a constant] field, and cannot be assigned to.

Or:

Cannot assign a value to the literal [or constant] field '%s'.

Or, simpler;

Literals and constants are not assignable: '%s'.

ensure that initonly fields are not set here

@baronfel, I thought that was already the case? In the linked issue, it's shown that such fields cannot be assigned to.

I agree it should be an error, apparently this is a condition protected by the CLR.

[cartermp]

• RFC/etc

Not necessary I think. Technically a change, but this is of the bugfix variety

• Other conditions

initonly fields as well, though it could be tricky since it's assignable during declaration or via a static constructor. If that doesn't apply (e.g., we're merely importing an existing field) then it should be straightforward

• Warnings

Yes, we'll need a new warning for each case. Should be "Cannot assign 'foo' to a field marked as literal". A warning for initonly is trickier since we don't really have that concept in F#.

[baronfel]

@abelbraaksma good eye on the already-handled initonly errors. That led me to believe my change here should be slightly refactored: instead of just checking in this one location, I should push the check into the BuildILFieldSet and BuildILStaticFieldSet functions in TypeChecker.fs, which are already doing the initonly check.

@cartermp As it is, I'm not sure that the compiler makes a distinction between those cases at the moment, from what I can see in BuildILFieldSet and BuildILStaticFieldSet, any presence of the initonly modifier causes an error to be reported.

Also @cartermp, you call out warning in your response. Can you share your rationale for warning vs error here?

[cartermp]

In this case it is technically a source breaking change. But I guess since it's a runtime error nobody is actually depending on being able to do this.

[baronfel]

I've made dedicated messages now, examples are in the issue submission. I went with @cartermp's phrasing, but quickly discovered that I needed different cases for when the expr about to be set on the literal was a Const expr vs non-const expr. For Const exprs we can write out the exact input in the message, but for non-const exprs we use a different formulation of the message. Feedback desired on both, if you please!

Also, any pointers as to where tests should go?

[baronfel]

Any ideas how I can verify that my newly-added tests ran? I've scoured the output and build logs but I'm not seeing any output at all for the tests I wrote, or event any tests for the other module in the same file (C# <-> F# basic interop).

Is the test signature of unit -> CompilationResult causing nunit not to discover these tests?

[baronfel]

I was able to invalidate my own assertion using dotnet test -t to list the tests, and I can see that the tests are listed. However I'm still not confident that these tests are being run in the CI setup. @vzarytovskii do you have any pointers I could use to verify if they are?

[vzarytovskii]

Any ideas how I can verify that my newly-added tests ran? I've scoured the output and build logs but I'm not seeing any output at all for the tests I wrote, or event any tests for the other module in the same file (C# <-> F# basic interop).

Is the test signature of unit -> CompilationResult causing nunit not to discover these tests?

We use xUnit instead in the ComponentTests, and it should be fine with such signature.

I'm not sure how xUnit handles several modules though. Let me quickly check.

[baronfel]

I agree on the signature, I need to rollback my most recent commit as a result

[vzarytovskii]

I was able to invalidate my own assertion using dotnet test -t to list the tests, and I can see that the tests are listed. However I'm still not confident that these tests are being run in the CI setup. @vzarytovskii do you have any pointers I could use to verify if they are?

@baronfel
I can't see this module running in the CI either, however, other modules in the same suite are fine.

I suspect it can be because in the fsproj file, this particular file is listed with different path separator "/" vs "\" elsewhere.

Maybe that's the reason test runner cannot run it. That would also explain why it runs just fine on my Linux laptop.

I guess I need to introduce some safety mechanism of how many tests were discovered vs how many were executed.

At least in CI.

[baronfel]

Good eye! I noticed that it ran (even though it immediately crashed) for me on MacOS as well. I'll make that change and see if the CI gets it.

[vzarytovskii]

Good eye! I noticed that it ran for me on MacOS as well. I'll make that change and see if the CI gets it.

Thanks, I will talk to test platform folks tomorrow and ask them if it's by design. I would expect it to be normalized.

Sorry, I overlooked it in the first place.

[vzarytovskii]

Good eye! I noticed that it ran (even though it immediately crashed) for me on MacOS as well. I'll make that change and see if the CI gets it.

You mean like hard crash? Was it inside compiler, or test framework?

[baronfel]

It was in the framework, but I had not done a proper rebuild via the shell script wrappers at the top-level of the repo. I was just using dotnet test in the new test Components folder as a shortcut. I am so used to local tests not working (on my MacOS machine) that I didn't even try any other method. Mostly I'm validating that the tests can be found locally and pushing them to CI for full validation.

[baronfel]

As a complete example, when I run dotnet test -f netcoreapp3.1 from the FSharp.Compiler.ComponentTests directory, with no other repo-wide building, I get the following error:

  X FSharp.Compiler.ErrorMessages.ComponentTests.C# <-> F# interop: fields.can't mutably set a C#-const field in F# [1ms]
  Error Message:
   System.TypeInitializationException : The type initializer for 'FSharp.Test.Utilities.CompilerAssert' threw an exception.
---- System.TypeInitializationException : The type initializer for '<StartupCode$FSharp-Test-Utilities>.$CompilerAssert' threw an exception.
-------- System.Exception : Couldn't find "PEVerify/Debug/netcoreapp3.1/PEVerify" on the following paths: "/Users/chethusk/oss/fsharp/tests/FSharp.Test.Utilities/../../artifacts/bin/PEVerify/Debug/netcoreapp3.1/PEVerify", "/Users/chethusk/oss/fsharp/tests/FSharp.Test.Utilities/../../artifacts/bin/peverify/debug/netcoreapp3.1/peverify". Running 'build test' once might solve this issue
  Stack Trace:
     at FSharp.Test.Utilities.CompilerAssert.CompileRaw(Compilation cmpl)
   at FSharp.Test.Utilities.Compiler.compileFSharpCompilation(Compilation compilation, Boolean ignoreWarnings) in /Users/chethusk/oss/fsharp/tests/FSharp.Test.Utilities/Compiler.fs:line 225
   at FSharp.Test.Utilities.Compiler.compileFSharp(FSharpCompilationSource fsSource) in /Users/chethusk/oss/fsharp/tests/FSharp.Test.Utilities/Compiler.fs:line 254
   at FSharp.Test.Utilities.Compiler.compile(CompilationUnit cUnit) in /Users/chethusk/oss/fsharp/tests/FSharp.Test.Utilities/Compiler.fs:line 306
   at FSharp.Compiler.ErrorMessages.ComponentTests.C# <-> F# interop: fields.can't mutably set a C#-const field in F#() in /Users/chethusk/oss/fsharp/tests/FSharp.Compiler.ComponentTests/Interop/SimpleInteropTests.fs:line 95
----- Inner Stack Trace -----
   at FSharp.Test.Utilities.CompilerAssert..cctor()
----- Inner Stack Trace -----
   at TestFramework.requireFile(String dir, String path) in /Users/chethusk/oss/fsharp/tests/FSharp.Test.Utilities/TestFramework.fs:line 198
   at TestFramework.requireArtifact@230.Invoke(String path) in /Users/chethusk/oss/fsharp/tests/FSharp.Test.Utilities/TestFramework.fs:line 230
   at TestFramework.config(String configurationName, FSharpMap`2 envVars) in /Users/chethusk/oss/fsharp/tests/FSharp.Test.Utilities/TestFramework.fs:line 240
   at TestFramework.initializeSuite() in /Users/chethusk/oss/fsharp/tests/FSharp.Test.Utilities/TestFramework.fs:line 343
   at <StartupCode$FSharp-Test-Utilities>.$CompilerAssert..cctor() in /Users/chethusk/oss/fsharp/tests/FSharp.Test.Utilities/CompilerAssert.fs:line 82

[vzarytovskii]

Ah, gotcha. I will need to have PEVerify built when Test Utilities are built as well.

I will create an issue for that and will fix, should be easy enough.

[baronfel]

Well, it's also not a huge deal to run ./build.sh --test once. Except for the part where build.sh --test fails on my machine due to missing assemblies (PresentationCore/PresentationFramework/a few others). :D

[vzarytovskii]

It seems, there's one more issue - the namespace is incorrect in this particular test file (ErrorMessages, as oppose to Interop), not sure if it adds to a problem.

Edit: yeah, I'm almost certain that the issue is in namespaces naming.
Should be in format of
"FSharp.Compiler.Category.ComponentTests"

For example
FSharp.Compiler.Language.ComponentTests
Or
FSharp.Compiler.Interop.ComponentTests

There are several occurrences of such tests. I will try to fix them later today or first thing tomorrow morning (I'm in CEST).

[baronfel]

I went ahead and did this file (so I could start testing my tests :) )

[baronfel]

I will need to have PEVerify built when Test Utilities are built as well.

One thing to note on the issue you create for this is that the path to the PEVerify binary needs to take into account that .Net Core builds .dll binaries, not .exe binaries, by default. I looked through the TestFramework's Config initialization to try and patch it locally so I could even run these tests, and there's a lot of implicit assumptions about being able to execute the files that are found, when in reality the pattern should be dotnet <path to dll> on .Net Core.

[vzarytovskii]

I will need to have PEVerify built when Test Utilities are built as well.

One thing to note on the issue you create for this is that the path to the PEVerify binary needs to take into account that .Net Core builds .dll binaries, not .exe binaries, by default. I looked through the TestFramework's Config initialization to try and patch it locally so I could even run these tests, and there's a lot of implicit assumptions about being able to execute the files that are found, when in reality the pattern should be dotnet <path to dll> on .Net Core.

Yeah, it has certain "history" :)
I plan to fix all those things once I get to IL and PE verification apis.

Since we want all (non-vs) tests to run anywhere, it will be a must.

[baronfel]

@vzarytovskii after making that namespace change, there still seems to be no change in the set of test run. What I'm doing is going to the Windows fsharpqa_release build, to the Build/Test step and searching the log for one of the names in this file, eg 'Instantiate'. This returns no results. Then I check the uploaded tests results display in the job to no avail either. I feel like I must yield the floor to you to see what precisely is going on here.

[vzarytovskii]

@vzarytovskii after making that namespace change, there still seems to be no change in the set of test run. What I'm doing is going to the Windows fsharpqa_release build, to the Build/Test step and searching the log for one of the names in this file, eg 'Instantiate'. This returns no results. Then I check the uploaded tests results display in the job to no avail either. I feel like I must yield the floor to you to see what precisely is going on here.

I usually go to Azure DevOps run, to tests tab, clear the filter and search for the test.
I can see your test there:

https://dev.azure.com/dnceng/public/_build/results?buildId=750200&view=ms.vss-test-web.build-test-results-tab&runId=23258676&resultId=100114&paneView=debug

I guess fsharpqa only runs the fsharpqa suite. But not others.

[baronfel]

I'm so confused, somehow searching for the exact fully-qualified test name didn't show when I tried the same as you. Yet searching for the word 'C#' did. Ah well, I'll take it either way 👍

@cartermp tests are in and green!

[cartermp]

Minor nits, but otherwise looks great!

[baronfel]

Made those changes, good to go.

[KevinRansom]

Thank you for this.