Skip to content

Commit

Permalink
Do not throw from IsUnionCaseTester and handle a case where symbol is…
Browse files Browse the repository at this point in the history 
… not considered a method/property (#17634)

* WIP some unit tests

* Return false instead of throwing

* Fix IsUnionCaseTester for generated code

Seems like methods/properties that are generated in IL, like `get_Is*`
in this case, have `IsMethod` (or `IsProperty`) false for some reason,
even when `IsPropertyGetterMethod` is true. This would result in
`IsUnionCaseTester` giving incorrect answers. This fixes that at
`IsUnionCaseTester`, though it might be worth it to see if it can be
fixed at the root of the issue

* Add a release note

* Move helpers around in unit tests for more reuse

* Add negative unit test

---------

Co-authored-by: Adam Boniecki <abonie@users.noreply.github.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
  • Loading branch information
@abonie @vzarytovskii
3 people authored Aug 30, 2024
1 parent 11561af commit 99bd001
Show file tree
Hide file tree
Showing 7 changed files with 235 additions and 79 deletions.
1 change: 1 addition & 0 deletions docs/release-notes/.FSharp.Compiler.Service/9.0.100.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* Fixed checking failure when `global` namespace is involved with enabled GraphBasedChecking ([PR #17553](https://github.com/dotnet/fsharp/pull/17553))
* Add missing byte chars notations, enforce limits in decimal notation in byte char & string (Issues [#15867](https://github.com/dotnet/fsharp/issues/15867), [#15868](https://github.com/dotnet/fsharp/issues/15868), [#15869](https://github.com/dotnet/fsharp/issues/15869), [PR #15898](https://github.com/dotnet/fsharp/pull/15898))
* Parentheses analysis: keep extra parentheses around unit & tuples in method definitions. ([PR #17618](https://github.com/dotnet/fsharp/pull/17618))
* Fix IsUnionCaseTester throwing for non-methods/properties [#17301](https://github.com/dotnet/fsharp/pull/17634)
* Consider `open type` used when the type is an enum and any of the enum cases is used unqualified. ([PR #17628](https://github.com/dotnet/fsharp/pull/17628))

### Added
Expand Down
6 changes: 5 additions & 1 deletion src/Compiler/Symbols/Symbols.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1788,7 +1788,11 @@ type FSharpMemberOrFunctionOrValue(cenv, d:FSharpMemberOrValData, item) =
match d with
| P p -> p.IsUnionCaseTester
| M m -> m.IsUnionCaseTester
| E _ | C _ | V _ -> invalidOp "the value or member is not a property"
| V v ->
v.IsPropertyGetterMethod &&
v.LogicalName.StartsWith("get_Is") &&
v.IsImplied && v.MemberApparentEntity.IsUnionTycon
| E _ | C _ -> false

member _.EventAddMethod =
checkIsResolved()
Expand Down
47 changes: 47 additions & 0 deletions tests/FSharp.Compiler.ComponentTests/Signatures/TypeTests.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,3 +263,50 @@ type Foo =
member Bar: a: int -> int with get
member Bar: a: int -> int with set"""

[<Fact>]
let ``get_Is* method has IsUnionCaseTester = true`` () =
FSharp """
module Lib
type Foo =
| Bar of int
| Baz of string
member this.IsP
with get () = 42
let bar = Bar 5
let f = bar.get_IsBar
"""
|> withLangVersionPreview
|> typecheckResults
|> fun results ->
let isBarSymbolUse = results.GetSymbolUseAtLocation(12, 21, "let f = bar.get_IsBar", [ "get_IsBar" ]).Value
match isBarSymbolUse.Symbol with
| :? FSharpMemberOrFunctionOrValue as mfv ->
Assert.True(mfv.IsUnionCaseTester, "IsUnionCaseTester returned true")
Assert.True(mfv.IsMethod, "IsMethod returned true")
Assert.False(mfv.IsProperty, "IsProptery returned true")
Assert.True(mfv.IsPropertyGetterMethod, "IsPropertyGetterMethod returned false")
| _ -> failwith "Expected FSharpMemberOrFunctionOrValue"

[<Fact>]
let ``IsUnionCaseTester does not throw for non-method non-property`` () =
FSharp """
module Lib
type Foo() =
member _.Bar x = x
let foo = Foo()
"""
|> withLangVersionPreview
|> typecheckResults
|> fun results ->
let isBarSymbolUse = results.GetSymbolUseAtLocation(7, 13, "let foo = Foo()", [ "Foo" ]).Value
match isBarSymbolUse.Symbol with
| :? FSharpMemberOrFunctionOrValue as mfv ->
Assert.False(mfv.IsUnionCaseTester, "IsUnionCaseTester returned true")
Assert.True(mfv.IsConstructor)
| _ -> failwith "Expected FSharpMemberOrFunctionOrValue"
62 changes: 61 additions & 1 deletion tests/FSharp.Compiler.Service.Tests/Common.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ open System
open System.Diagnostics
open System.IO
open System.Collections.Generic
open System.Threading
open System.Threading.Tasks
open FSharp.Compiler.CodeAnalysis
open FSharp.Compiler.IO
open FSharp.Compiler.Diagnostics
open FSharp.Compiler.Symbols
open FSharp.Compiler.Syntax
open FSharp.Compiler.Text
Expand Down Expand Up @@ -473,3 +473,63 @@ let assertRange
Assert.Equal(Position.mkPos expectedStartLine expectedStartColumn, actualRange.Start)
Assert.Equal(Position.mkPos expectedEndLine expectedEndColumn, actualRange.End)

[<AutoOpen>]
module TempDirUtils =
let getTempPath dir =
Path.Combine(Path.GetTempPath(), dir)

/// Returns the file name part of a temp file name created with tryCreateTemporaryFileName ()
/// and an added process id and thread id to ensure uniqueness between threads.
let getTempFileName() =
let tempFileName = tryCreateTemporaryFileName ()
try
let tempFile, tempExt = Path.GetFileNameWithoutExtension tempFileName, Path.GetExtension tempFileName
let procId, threadId = Process.GetCurrentProcess().Id, Thread.CurrentThread.ManagedThreadId
String.concat "" [tempFile; "_"; string procId; "_"; string threadId; tempExt] // ext includes dot
finally
try
FileSystem.FileDeleteShim tempFileName
with _ -> ()

/// Given just a file name, returns it with changed extension located in %TEMP%\ExprTests
let getTempFilePathChangeExt dir tmp ext =
Path.Combine(getTempPath dir, Path.ChangeExtension(tmp, ext))

/// If it doesn't exists, create a folder 'ExprTests' in local user's %TEMP% folder
let createTempDir dirName =
let tempPath = getTempPath dirName
do
if Directory.Exists tempPath then ()
else Directory.CreateDirectory tempPath |> ignore

/// Clean up after a test is run. If you need to inspect the create *.fs files, change this function to do nothing, or just break here.
let cleanupTempFiles dirName files =
{ new IDisposable with
member _.Dispose() =
for fileName in files do
try
// cleanup: only the source file is written to the temp dir.
FileSystem.FileDeleteShim fileName
with _ -> ()

try
// remove the dir when empty
let tempPath = getTempPath dirName
if Directory.GetFiles tempPath |> Array.isEmpty then
Directory.Delete tempPath
with _ -> () }

let createProjectOptions dirName fileSources extraArgs =
let fileNames = fileSources |> List.map (fun _ -> getTempFileName())
let temp2 = getTempFileName()
let fileNames = fileNames |> List.map (fun temp1 -> getTempFilePathChangeExt dirName temp1 ".fs")
let dllName = getTempFilePathChangeExt dirName temp2 ".dll"
let projFileName = getTempFilePathChangeExt dirName temp2 ".fsproj"

createTempDir dirName
for fileSource: string, fileName in List.zip fileSources fileNames do
FileSystem.OpenFileForWriteShim(fileName).Write(fileSource)
let args = [| yield! extraArgs; yield! mkProjectCommandLineArgs (dllName, []) |]
let options = { checker.GetProjectOptionsFromCommandLineArgs (projFileName, args) with SourceFiles = fileNames |> List.toArray }

cleanupTempFiles dirName (fileNames @ [dllName; projFileName]), options
93 changes: 16 additions & 77 deletions tests/FSharp.Compiler.Service.Tests/ExprTests.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,14 @@
open Xunit
open FsUnit
open System
open System.IO
open System.Text
open System.Collections.Generic
open System.Diagnostics
open System.Threading
open FSharp.Compiler.CodeAnalysis
open FSharp.Compiler.Diagnostics
open FSharp.Compiler.IO
open FSharp.Compiler.Service.Tests.Common
open FSharp.Compiler.Symbols
open FSharp.Compiler.Symbols.FSharpExprPatterns
open TestFramework

type FSharpCore =
| FC45
Expand All @@ -29,52 +25,11 @@ type FSharpCore =
| FC47 -> "FSharp.Core 4.7"
| FC50 -> "FSharp.Core 5.0"

[<Literal>]
let dirName = "ExprTests"

[<AutoOpen>]
module internal Utils =
let getTempPath() =
Path.Combine(Path.GetTempPath(), "ExprTests")

/// If it doesn't exists, create a folder 'ExprTests' in local user's %TEMP% folder
let createTempDir() =
let tempPath = getTempPath()
do
if Directory.Exists tempPath then ()
else Directory.CreateDirectory tempPath |> ignore

/// Returns the file name part of a temp file name created with tryCreateTemporaryFileName ()
/// and an added process id and thread id to ensure uniqueness between threads.
let getTempFileName() =
let tempFileName = tryCreateTemporaryFileName ()
try
let tempFile, tempExt = Path.GetFileNameWithoutExtension tempFileName, Path.GetExtension tempFileName
let procId, threadId = Process.GetCurrentProcess().Id, Thread.CurrentThread.ManagedThreadId
String.concat "" [tempFile; "_"; string procId; "_"; string threadId; tempExt] // ext includes dot
finally
try
FileSystem.FileDeleteShim tempFileName
with _ -> ()

/// Clean up after a test is run. If you need to inspect the create *.fs files, change this function to do nothing, or just break here.
let cleanupTempFiles files =
{ new IDisposable with
member _.Dispose() =
for fileName in files do
try
// cleanup: only the source file is written to the temp dir.
FileSystem.FileDeleteShim fileName
with _ -> ()

try
// remove the dir when empty
let tempPath = getTempPath()
if Directory.GetFiles tempPath |> Array.isEmpty then
Directory.Delete tempPath
with _ -> () }

/// Given just a file name, returns it with changed extension located in %TEMP%\ExprTests
let getTempFilePathChangeExt tmp ext =
Path.Combine(getTempPath(), Path.ChangeExtension(tmp, ext))

// This behaves slightly differently on Mono versions, 'null' is printed sometimes, 'None' other times
// Presumably this is very small differences in Mono reflection causing F# printing to change behaviour
Expand Down Expand Up @@ -345,22 +300,6 @@ module internal Utils =
yield! collectMembers e |> Seq.map printMemberSignature
}


let createOptionsAux fileSources extraArgs =
let fileNames = fileSources |> List.map (fun _ -> Utils.getTempFileName())
let temp2 = Utils.getTempFileName()
let fileNames = fileNames |> List.map (fun temp1 -> Utils.getTempFilePathChangeExt temp1 ".fs")
let dllName = Utils.getTempFilePathChangeExt temp2 ".dll"
let projFileName = Utils.getTempFilePathChangeExt temp2 ".fsproj"

Utils.createTempDir()
for fileSource: string, fileName in List.zip fileSources fileNames do
FileSystem.OpenFileForWriteShim(fileName).Write(fileSource)
let args = [| yield! extraArgs; yield! mkProjectCommandLineArgs (dllName, []) |]
let options = { checker.GetProjectOptionsFromCommandLineArgs (projFileName, args) with SourceFiles = fileNames |> List.toArray }

Utils.cleanupTempFiles (fileNames @ [dllName; projFileName]), options

//---------------------------------------------------------------------------------------------------------
// This project is a smoke test for a whole range of standard and obscure expressions

Expand Down Expand Up @@ -653,7 +592,7 @@ let testMutableVar = mutableVar 1
let testMutableConst = mutableConst ()
"""

let createOptionsWithArgs args = createOptionsAux [ fileSource1; fileSource2 ] args
let createOptionsWithArgs args = createProjectOptions dirName [ fileSource1; fileSource2 ] args

let createOptions() = createOptionsWithArgs []

Expand Down Expand Up @@ -1002,15 +941,15 @@ let ``Test Optimized Declarations Project1`` useTransparentCompiler =

let testOperators dnName fsName excludedTests expectedUnoptimized expectedOptimized =

let tempFileName = Utils.getTempFileName()
let filePath = Utils.getTempFilePathChangeExt tempFileName ".fs"
let dllPath =Utils.getTempFilePathChangeExt tempFileName ".dll"
let projFilePath = Utils.getTempFilePathChangeExt tempFileName ".fsproj"
let tempFileName = getTempFileName()
let filePath = getTempFilePathChangeExt dirName tempFileName ".fs"
let dllPath =getTempFilePathChangeExt dirName tempFileName ".dll"
let projFilePath = getTempFilePathChangeExt dirName tempFileName ".fsproj"
let exprChecker = FSharpChecker.Create(keepAssemblyContents=true, useTransparentCompiler=true)

begin
use _cleanup = Utils.cleanupTempFiles [filePath; dllPath; projFilePath]
createTempDir()
use _cleanup = cleanupTempFiles dirName [filePath; dllPath; projFilePath]
createTempDir dirName
let source = String.Format(Project1.operatorTests, dnName, fsName)
let replace (s:string) r = s.Replace("let " + r, "// let " + r)
let fileSource = excludedTests |> List.fold replace source
Expand Down Expand Up @@ -3192,7 +3131,7 @@ let BigSequenceExpression(outFileOpt,docFileOpt,baseAddressOpt) =
"""


let createOptions() = createOptionsAux [fileSource1] []
let createOptions() = createProjectOptions dirName [fileSource1] []

#if !NETFRAMEWORK && DEBUG
[<Fact(Skip = "Test is known to fail in DEBUG when not using NetFramework. Use RELEASE configuration or NetFramework to run it.")>]
Expand Down Expand Up @@ -3279,7 +3218,7 @@ let f7() = callXY (C()) (D())
let f8() = callXY (D()) (C())
"""

let createOptions() = createOptionsAux [fileSource1] ["--langversion:7.0"]
let createOptions() = createProjectOptions dirName [fileSource1] ["--langversion:7.0"]

[<Theory>]
[<InlineData(false)>]
Expand Down Expand Up @@ -3407,7 +3346,7 @@ type MyNumberWrapper =
{ MyNumber: MyNumber }
"""

let createOptions() = createOptionsAux [fileSource1] ["--langversion:7.0"]
let createOptions() = createProjectOptions dirName [fileSource1] ["--langversion:7.0"]

[<Theory>]
[<InlineData(false)>]
Expand Down Expand Up @@ -3465,13 +3404,13 @@ let s2 = sign p1
"""

let createOptions() = createOptionsAux [fileSource1] ["--langversion:7.0"]
let createOptions() = createProjectOptions dirName [fileSource1] ["--langversion:7.0"]

[<Theory>]
[<InlineData(false)>]
[<InlineData(true)>]
let ``Test ProjectForWitnesses3`` useTransparentCompiler =
let cleanup, options = createOptionsAux [ ProjectForWitnesses3.fileSource1 ] ["--langversion:7.0"]
let cleanup, options = createProjectOptions dirName [ ProjectForWitnesses3.fileSource1 ] ["--langversion:7.0"]
use _holder = cleanup
let exprChecker = FSharpChecker.Create(keepAssemblyContents=true, useTransparentCompiler=useTransparentCompiler)
let wholeProjectResults = exprChecker.ParseAndCheckProject(options) |> Async.RunImmediate
Expand Down Expand Up @@ -3563,7 +3502,7 @@ let isNullQuoted (ts : 't[]) =
"""

let createOptions() = createOptionsAux [fileSource1] ["--langversion:7.0"]
let createOptions() = createProjectOptions dirName [fileSource1] ["--langversion:7.0"]

[<Theory>]
[<InlineData(false)>]
Expand Down Expand Up @@ -3603,7 +3542,7 @@ module N.M
let rec f = new System.EventHandler(fun _ _ -> f.Invoke(null,null))
"""

let createOptions() = createOptionsAux [fileSource1] []
let createOptions() = createProjectOptions dirName [fileSource1] []

[<Theory>]
[<InlineData(false)>]
Expand Down
1 change: 1 addition & 0 deletions tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
<Link>FsUnit.fs</Link>
</Compile>
<Compile Include="Common.fs" />
<Compile Include="GeneratedCodeSymbolsTests.fs" />
<Compile Include="AssemblyReaderShim.fs" />
<Compile Include="ModuleReaderCancellationTests.fs" />
<Compile Include="EditorTests.fs" />
Expand Down
Loading

0 comments on commit 99bd001

Please sign in to comment.