Skip to content

Commit

Permalink
Unsafe option unwrapping analyzer
Browse files Browse the repository at this point in the history
Generalizes the existing analyzer to cover ValueOption and `.Value`
member access
  • Loading branch information
sheridanchris committed Nov 15, 2023
1 parent ab29042 commit 360d688
Show file tree
Hide file tree
Showing 7 changed files with 137 additions and 72 deletions.
7 changes: 4 additions & 3 deletions docs/suggestion/006.md
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
---
title: ReplaceOptionGetWithGracefulHandling
title: ReplaceOptionUnwrapWithGracefulHandling
category: suggestion
categoryindex: 2
index: 5
---
# OptionGetAnalyzer
# HandleOptionGracefullyAnalyzer

## Problem
The option type is used to model a potentially missing value. Unwrapping an `'a option` into an `'a` by using `Option.get` circumvents the graceful handling of the `None` case. You should unwrap the value using a function like `Option.defaultValue` or pattern matching instead.
The option type is used to model a potentially missing value. Unwrapping an `'a option` into an `'a` by using `Option.get` or `option.Value` circumvents the graceful handling of the `None` case. You should unwrap the value using a function like `Option.defaultValue` or pattern matching instead.

```fsharp
let option = None
let value = Option.get option // Triggers analyzer
let value' = option.Value // Triggers analyzer
```

## Fix
Expand Down
2 changes: 1 addition & 1 deletion src/Ionide.Analyzers/Ionide.Analyzers.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
<Compile Include="Suggestion\IgnoreFunctionAnalyzer.fs"/>
<Compile Include="Suggestion\UnnamedDiscriminatedUnionFieldAnalyzer.fs"/>
<Compile Include="Suggestion\EmptyStringAnalyzer.fs"/>
<Compile Include="Suggestion\ReplaceOptionGetWithGracefulHandlingAnalyzer.fs"/>
<Compile Include="Suggestion\HandleOptionGracefullyAnalyzer.fs"/>
<Compile Include="Style\SquareBracketArrayAnalyzer.fs"/>
</ItemGroup>

Expand Down
45 changes: 45 additions & 0 deletions src/Ionide.Analyzers/Suggestion/HandleOptionGracefullyAnalyzer.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
module Ionide.Analyzers.Suggestion.HandleOptionGracefullyAnalyzer

open System
open FSharp.Analyzers.SDK
open FSharp.Analyzers.SDK.TASTCollecting
open FSharp.Compiler.Symbols
open FSharp.Compiler.Text

[<CliAnalyzer("HandleOptionGracefullyAnalyzer",
"Replace unsafe option unwrapping with graceful handling of each case.",
"https://ionide.io/ionide-analyzers/suggestion/006.html")>]
let optionGetAnalyzer (ctx: CliContext) =
async {
let messages = ResizeArray<Message>()

let walker =
{ new TypedTreeCollectorBase() with
override x.WalkCall _ (mfv: FSharpMemberOrFunctionOrValue) _ _ (args: FSharpExpr list) (m: range) =
let fullyQualifiedCall =
String.Join(".", mfv.DeclaringEntity.Value.FullName, mfv.DisplayName)

if
(mfv.FullName = "Microsoft.FSharp.Core.Option.get"
|| mfv.FullName = "Microsoft.FSharp.Core.ValueOption.get"
|| fullyQualifiedCall = "Microsoft.FSharp.Core.FSharpOption`1.Value"
|| fullyQualifiedCall = "Microsoft.FSharp.Core.FSharpValueOption`1.Value")
&& args.Length = 1
then
messages.Add
{
Type = "HandleOptionGracefully"
Message = "Replace unsafe option unwrapping with graceful handling of each case."
Code = "IONIDE-006"
Severity = Severity.Hint
Range = m
Fixes = []
}
}

match ctx.TypedTree with
| None -> return []
| Some typedTree ->
walkTast walker typedTree
return Seq.toList messages
}

This file was deleted.

2 changes: 1 addition & 1 deletion tests/Ionide.Analyzers.Tests/Ionide.Analyzers.Tests.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<Compile Include="Suggestion\IgnoreFunctionAnalyzerTests.fs" />
<Compile Include="Suggestion\UnnamedDiscriminatedUnionFieldAnalyzerTests.fs" />
<Compile Include="Suggestion\EmptyStringAnalyzerTests.fs" />
<Compile Include="Suggestion\ReplaceOptionGetWithGracefulHandlingAnalyzerTests.fs" />
<Compile Include="Suggestion\HandleOptionGracefullyAnalyzerTests.fs" />
<Compile Include="Style\SquareBracketArrayAnalyzerTests.fs" />
<Compile Include="Program.fs" />
</ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
module Ionide.Analyzers.Tests.Suggestion.HandleOptionGracefullyAnalyzerTests

open NUnit.Framework
open FSharp.Compiler.CodeAnalysis
open FSharp.Analyzers.SDK.Testing
open Ionide.Analyzers.Suggestion.HandleOptionGracefullyAnalyzer

let mutable projectOptions: FSharpProjectOptions = FSharpProjectOptions.zero

let messageString =
"Replace unsafe option unwrapping with graceful handling of each case."

[<SetUp>]
let Setup () =
task {
let! opts = mkOptionsFromProject "net7.0" []
projectOptions <- opts
}

[<Test>]
let ``Option.get is detected`` () =
async {
let source =
"""
module M
let option = Some 10
let value = Option.get option
"""

let ctx = getContext projectOptions source
let! msgs = optionGetAnalyzer ctx
Assert.IsNotEmpty msgs
Assert.IsTrue(Assert.messageContains messageString msgs[0])
}

[<Test>]
let ``ValueOption.get is detected`` () =
async {
let source =
"""
module M
let voption = ValueSome 10
let value = ValueOption.get voption
"""

let ctx = getContext projectOptions source
let! msgs = optionGetAnalyzer ctx
Assert.IsNotEmpty msgs
Assert.IsTrue(Assert.messageContains messageString msgs[0])
}

[<Test>]
let ``Option.Value member is detected`` () =
async {
let source =
"""
module M
let option = Some 10
let value = option.Value
"""

let ctx = getContext projectOptions source
let! msgs = optionGetAnalyzer ctx
Assert.IsNotEmpty msgs
Assert.IsTrue(Assert.messageContains messageString msgs[0])
}

[<Test>]
let ``ValueOption.Value member is detected`` () =
async {
let source =
"""
module M
let voption = ValueSome 10
let value = voption.Value
"""

let ctx = getContext projectOptions source
let! msgs = optionGetAnalyzer ctx
Assert.IsNotEmpty msgs
Assert.IsTrue(Assert.messageContains messageString msgs[0])
}

This file was deleted.

0 comments on commit 360d688

Please sign in to comment.