diff --git a/docs/content/how-tos/rule-configuration.md b/docs/content/how-tos/rule-configuration.md index 40ed37fb6..2369d51ed 100644 --- a/docs/content/how-tos/rule-configuration.md +++ b/docs/content/how-tos/rule-configuration.md @@ -123,3 +123,4 @@ The following rules can be specified for linting. - [UnnestedFunctionNames (FL0080)](rules/FL0080.html) - [NestedFunctionNames (FL0081)](rules/FL0081.html) - [UsedUnderscorePrefixedElements (FL0082)](rules/FL0082.html) +- [FavourNonMutablePropertyInitialization (FL0083)](rules/FL0083.html) diff --git a/docs/content/how-tos/rules/FL0083.md b/docs/content/how-tos/rules/FL0083.md new file mode 100644 index 000000000..8877acf52 --- /dev/null +++ b/docs/content/how-tos/rules/FL0083.md @@ -0,0 +1,29 @@ +--- +title: FL0083 +category: how-to +hide_menu: true +--- + +# FavourNonMutablePropertyInitialization (FL0077) + +*Introduced in `0.23.2`* + +## Cause + +Use of mutation to initialize properties with values. + +## Rationale + +There's no need to use mutation to initialize properties with values because F# has specific syntax for this kind of intitialization. +Both approaches will compile to the same IL but non-mutable code doesn't look unsafe because it prevents the use of the `<-` operator. + +## How To Fix + +Replace all occurrences of mutable property initialization with non mutable property initialization. +Example: `Cookie(Domain = "example.com")` instead of `let c = Cookie(); c.Domain <- "example.com"` + +## Rule Settings + + { + "FavourNonMutablePropertyInitialization": { "enabled": false } + } diff --git a/src/FSharpLint.Core/Application/Configuration.fs b/src/FSharpLint.Core/Application/Configuration.fs index b3e23affd..66b507295 100644 --- a/src/FSharpLint.Core/Application/Configuration.fs +++ b/src/FSharpLint.Core/Application/Configuration.fs @@ -48,10 +48,7 @@ module FSharpJsonConverter = |] let serializerSettings = - let settings = JsonSerializerSettings() - settings.NullValueHandling <- NullValueHandling.Ignore - settings.Converters <- converters - settings + JsonSerializerSettings(NullValueHandling = NullValueHandling.Ignore, Converters = converters) module IgnoreFiles = @@ -310,6 +307,7 @@ type ConventionsConfig = redundantNewKeyword:EnabledConfig option favourStaticEmptyFields:EnabledConfig option asyncExceptionWithoutReturn:EnabledConfig option + favourNonMutablePropertyInitialization:EnabledConfig option nestedStatements:RuleConfig option cyclomaticComplexity:RuleConfig option reimplementsFunction:EnabledConfig option @@ -330,6 +328,7 @@ with this.recursiveAsyncFunction |> Option.bind (constructRuleIfEnabled RecursiveAsyncFunction.rule) |> Option.toArray this.avoidTooShortNames |> Option.bind (constructRuleIfEnabled AvoidTooShortNames.rule) |> Option.toArray this.redundantNewKeyword |> Option.bind (constructRuleIfEnabled RedundantNewKeyword.rule) |> Option.toArray + this.favourNonMutablePropertyInitialization |> Option.bind (constructRuleIfEnabled FavourNonMutablePropertyInitialization.rule) |> Option.toArray this.favourReRaise |> Option.bind (constructRuleIfEnabled FavourReRaise.rule) |> Option.toArray this.favourStaticEmptyFields |> Option.bind (constructRuleIfEnabled FavourStaticEmptyFields.rule) |> Option.toArray this.asyncExceptionWithoutReturn |> Option.bind (constructRuleIfEnabled AsyncExceptionWithoutReturn.rule) |> Option.toArray @@ -403,6 +402,7 @@ type Configuration = RecursiveAsyncFunction:EnabledConfig option AvoidTooShortNames:EnabledConfig option RedundantNewKeyword:EnabledConfig option + FavourNonMutablePropertyInitialization:EnabledConfig option FavourReRaise:EnabledConfig option FavourStaticEmptyFields:EnabledConfig option AsyncExceptionWithoutReturn:EnabledConfig option @@ -491,6 +491,7 @@ with RecursiveAsyncFunction = None AvoidTooShortNames = None RedundantNewKeyword = None + FavourNonMutablePropertyInitialization = None FavourReRaise = None FavourStaticEmptyFields = None AsyncExceptionWithoutReturn = None @@ -642,6 +643,7 @@ let flattenConfig (config:Configuration) = config.RecursiveAsyncFunction |> Option.bind (constructRuleIfEnabled RecursiveAsyncFunction.rule) config.AvoidTooShortNames |> Option.bind (constructRuleIfEnabled AvoidTooShortNames.rule) config.RedundantNewKeyword |> Option.bind (constructRuleIfEnabled RedundantNewKeyword.rule) + config.FavourNonMutablePropertyInitialization |> Option.bind (constructRuleIfEnabled FavourNonMutablePropertyInitialization.rule) config.FavourReRaise |> Option.bind (constructRuleIfEnabled FavourReRaise.rule) config.FavourStaticEmptyFields |> Option.bind (constructRuleIfEnabled FavourStaticEmptyFields.rule) config.AsyncExceptionWithoutReturn |> Option.bind (constructRuleIfEnabled AsyncExceptionWithoutReturn.rule) diff --git a/src/FSharpLint.Core/Application/Lint.fs b/src/FSharpLint.Core/Application/Lint.fs index c5072fd9a..baf829288 100644 --- a/src/FSharpLint.Core/Application/Lint.fs +++ b/src/FSharpLint.Core/Application/Lint.fs @@ -261,17 +261,18 @@ module Lint = ReachedEnd(fileInfo.File, fileWarnings |> Seq.toList) |> lintInfo.ReportLinterProgress let private runProcess (workingDir:string) (exePath:string) (args:string) = - let psi = System.Diagnostics.ProcessStartInfo() - psi.FileName <- exePath - psi.WorkingDirectory <- workingDir - psi.RedirectStandardOutput <- true - psi.RedirectStandardError <- true - psi.Arguments <- args - psi.CreateNoWindow <- true - psi.UseShellExecute <- false - - use p = new System.Diagnostics.Process() - p.StartInfo <- psi + let psi = + System.Diagnostics.ProcessStartInfo( + FileName = exePath, + WorkingDirectory = workingDir, + RedirectStandardOutput = true, + RedirectStandardError = true, + Arguments = args, + CreateNoWindow = true, + UseShellExecute = false + ) + + use p = new System.Diagnostics.Process(StartInfo = psi) let sbOut = System.Text.StringBuilder() p.OutputDataReceived.Add(fun ea -> sbOut.AppendLine(ea.Data) |> ignore) let sbErr = System.Text.StringBuilder() diff --git a/src/FSharpLint.Core/FSharpLint.Core.fsproj b/src/FSharpLint.Core/FSharpLint.Core.fsproj index 10e4c4345..95bc8b06d 100644 --- a/src/FSharpLint.Core/FSharpLint.Core.fsproj +++ b/src/FSharpLint.Core/FSharpLint.Core.fsproj @@ -54,6 +54,7 @@ + diff --git a/src/FSharpLint.Core/Rules/Conventions/FavourNonMutablePropertyInitialization.fs b/src/FSharpLint.Core/Rules/Conventions/FavourNonMutablePropertyInitialization.fs new file mode 100644 index 000000000..23e9dcd65 --- /dev/null +++ b/src/FSharpLint.Core/Rules/Conventions/FavourNonMutablePropertyInitialization.fs @@ -0,0 +1,92 @@ +module FSharpLint.Rules.FavourNonMutablePropertyInitialization + +open FSharpLint.Framework +open FSharpLint.Framework.Suggestion +open FSharp.Compiler.Syntax +open FSharpLint.Framework.Ast +open FSharpLint.Framework.Rules +open System + +let private getWarningDetails (ident: Ident) = + let formatError errorName = + String.Format(Resources.GetString errorName, ident.idText) + + "RulesFavourNonMutablePropertyInitializationError" + |> formatError + |> Array.singleton + |> Array.map (fun message -> + { Range = ident.idRange + Message = message + SuggestedFix = None + TypeChecks = List.Empty }) + +let private extraInstanceMethod (app:SynExpr) (instanceMethodCalls: List) = + match app with + | SynExpr.App(_, _, expression, _, _) -> + match expression with + | SynExpr.LongIdent(_, LongIdentWithDots(identifiers, _), _, _) -> + match List.tryLast identifiers with + | Some _ -> + identifiers.[0].idText::instanceMethodCalls + | _ -> instanceMethodCalls + | _ -> instanceMethodCalls + | _ -> instanceMethodCalls + +let rec private extraFromBindings (bindings: List) (classInstances: List) = + match bindings with + | SynBinding(_, _, _, _, _, _, _, SynPat.Named(_, ident, _, _, _), _, _expression, _, _)::rest -> + extraFromBindings rest (ident.idText::classInstances) + | _ -> classInstances + +let rec private processLetBinding (instanceNames: Set) (body: SynExpr) : array = + match body with + | SynExpr.LongIdentSet(LongIdentWithDots(identifiers, _), _, _) -> + match identifiers with + | [instanceIdent; propertyIdent] when Set.contains instanceIdent.idText instanceNames -> + getWarningDetails propertyIdent + | _ -> Array.empty + | SynExpr.Sequential(_, _, expr1, expr2, _) -> + let instanceNames = + Set.difference + instanceNames + (extraInstanceMethod expr1 List.empty |> Set.ofList) + Array.append + (processLetBinding instanceNames expr1) + (processLetBinding instanceNames expr2) + | _ -> [||] + +and processExpression (expression: SynExpr) : array = + match expression with + | SynExpr.LetOrUse(_, _, bindings, body, _) -> + let instanceNames = extraFromBindings bindings [] |> Set.ofList + processLetBinding instanceNames body + | SynExpr.Sequential(_, _, expr1, expr2, _) -> + Array.append + (processExpression expr1) + (processExpression expr2) + | _ -> Array.empty + +let runner args = + match args.AstNode with + | Binding(SynBinding(_, _, _, _, _, _, _, _, _, SynExpr.LetOrUse(_, _, bindings, body, _), _, _)) -> + let instanceNames = extraFromBindings bindings [] |> Set.ofList + processLetBinding instanceNames body + | Match(SynMatchClause(_, _, expr, _, _)) -> + processExpression expr + | Lambda(lambda, _) -> + processExpression lambda.Body + | Expression(SynExpr.TryWith(tryExpr, _, _, _, _, _, _)) -> + processExpression tryExpr + | Expression(SynExpr.TryFinally(tryExpr, finallyExpr, _, _, _)) -> + Array.append + (processExpression tryExpr) + (processExpression finallyExpr) + | Expression(SynExpr.CompExpr(_, _, expr, _)) -> + processExpression expr + | _ -> Array.empty + +let rule = + { Name = "FavourNonMutablePropertyInitialization" + Identifier = Identifiers.FavourNonMutablePropertyInitialization + RuleConfig = { AstNodeRuleConfig.Runner = runner; Cleanup = ignore } } + |> AstNodeRule diff --git a/src/FSharpLint.Core/Rules/Identifiers.fs b/src/FSharpLint.Core/Rules/Identifiers.fs index 16b4c86a1..5e6335cb9 100644 --- a/src/FSharpLint.Core/Rules/Identifiers.fs +++ b/src/FSharpLint.Core/Rules/Identifiers.fs @@ -86,4 +86,5 @@ let AsyncExceptionWithoutReturn = identifier 78 let SuggestUseAutoProperty = identifier 79 let UnnestedFunctionNames = identifier 80 let NestedFunctionNames = identifier 81 -let UsedUnderscorePrefixedElements = identifier 82 \ No newline at end of file +let UsedUnderscorePrefixedElements = identifier 82 +let FavourNonMutablePropertyInitialization = identifier 83 diff --git a/src/FSharpLint.Core/Text.resx b/src/FSharpLint.Core/Text.resx index 108959a45..7c8aa17bd 100644 --- a/src/FSharpLint.Core/Text.resx +++ b/src/FSharpLint.Core/Text.resx @@ -357,4 +357,7 @@ Consider using auto-properties via the 'val' keyword. + + Consider using non-mutable property initialization. + diff --git a/src/FSharpLint.Core/fsharplint.json b/src/FSharpLint.Core/fsharplint.json index dfe9984cd..32e0c556b 100644 --- a/src/FSharpLint.Core/fsharplint.json +++ b/src/FSharpLint.Core/fsharplint.json @@ -281,6 +281,7 @@ "uselessBinding": { "enabled": true }, "tupleOfWildcards": { "enabled": true }, "favourTypedIgnore": { "enabled": false }, + "favourNonMutablePropertyInitialization": { "enabled": true }, "favourReRaise": { "enabled": true }, "favourStaticEmptyFields": { "enabled": false }, "favourConsistentThis": { diff --git a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj index ec026fdd8..eb5f9361b 100644 --- a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj +++ b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj @@ -43,6 +43,7 @@ + diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/FavourNonMutablePropertyInitialization.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/FavourNonMutablePropertyInitialization.fs new file mode 100644 index 000000000..4df6d0073 --- /dev/null +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/FavourNonMutablePropertyInitialization.fs @@ -0,0 +1,264 @@ +module FSharpLint.Core.Tests.Rules.Conventions.FavourNonMutablePropertyInitialization + +open NUnit.Framework +open FSharpLint.Rules + +[] +type TestConventionsFavourNonMutablePropertyInitialization() = + inherit TestAstNodeRuleBase.TestAstNodeRuleBase(FavourNonMutablePropertyInitialization.rule) + + [] + member this.FavourNonMutablePropertyInitializationShouldProduceError_1() = + this.Parse """ +type SomeClass() = + let mutable myInternalValue = 1 + // A write-only property. + member this.MyWriteOnlyProperty with set (value) = myInternalValue <- value + +module Program = + let someFunction() = + let someInstance = SomeClass() + someInstance.MyWriteOnlyProperty <- 2""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.FavourNonMutablePropertyInitializationShouldProduceError_2() = + this.Parse """ +type SomeClass() = + let mutable myInternalValue = 1 + // A read-write property. + member this.MyReadWriteProperty + with get () = myInternalValue + and set (value) = myInternalValue <- value + +module Program = + let someFunction() = + let someInstance = SomeClass() + someInstance.MyReadWriteProperty <- 2""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.FavourNonMutablePropertyInitializationShouldProduceError_3() = + this.Parse """ +open System.Net + +module Program = + let someFunction() = + // System.Net.Cookie implementation lives in a referenced assembly + let someInstance = Cookie() + someInstance.Domain <- "example.com" +""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.FavourNonMutablePropertyInitializationShouldProduceError_4() = + this.Parse """ +type SomeClass() = + let mutable myInternalValue = 1 + static member SomeStaticMethod() = + () // any code goes here + // A read-write property. + member this.MyReadWriteProperty + with get () = myInternalValue + and set (value) = myInternalValue <- value + +module Program = + let someFunction() = + let someInstance = SomeClass() + SomeClass.SomeStaticMethod() + someInstance.MyReadWriteProperty <- 2""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.FavourNonMutablePropertyInitializationShouldProduceError_5() = + this.Parse """ +type SomeClass() = + let mutable myInternalValue = 1 + // A write-only property. + member this.MyWriteOnlyProperty with set (value) = myInternalValue <- value + +module Program = + let someFunction() = + let someInstance = SomeClass() + someInstance.MyWriteOnlyProperty <- 2 + ()""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.FavourNonMutablePropertyInitializationShouldProduceError_6() = + this.Parse """ +type SomeClass() = + member val SomeProperty1 = 0 with get, set + member val SomeProperty2 = 10 with get, set + +module Program = + let someFunction = + let someInstance = SomeClass() + someInstance.SomeProperty1 <- 2 + someInstance.SomeProperty2 <- 3""" + + Assert.IsTrue <| this.ErrorExistsAt(9, 21) + Assert.IsTrue <| this.ErrorExistsAt(10, 21) + + [] + member this.``FavourNonMutablePropertyInitialization should produce error in match expression``() = + this.Parse """ +type SomeClass() = + member val MyWriteOnlyProperty = 0 with set + +let someValue = + match () with + | _ -> + let someInstance = SomeClass() + someInstance.MyWriteOnlyProperty <- 2""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``FavourNonMutablePropertyInitialization should produce error in lambda function``() = + this.Parse """ +type SomeClass() = + member val MyWriteOnlyProperty = 0 with set + +let someLambda = + fun x -> + let someInstance = SomeClass() + someInstance.MyWriteOnlyProperty <- 2""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``FavourNonMutablePropertyInitialization should produce error in try-with block``() = + this.Parse """ +type SomeClass() = + member val MyWriteOnlyProperty = 0 with set + +let someValue = + try + let someInstance = SomeClass() + someInstance.MyWriteOnlyProperty <- 2 + with + | _ -> ()""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``FavourNonMutablePropertyInitialization should produce error in finally block``() = + this.Parse """ +type SomeClass() = + member val MyWriteOnlyProperty = 0 with set + +let someValue = + try + () + finally + let someInstance = SomeClass() + someInstance.MyWriteOnlyProperty <- 2""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``FavourNonMutablePropertyInitialization should produce error in computation expression``() = + this.Parse """ +type SomeClass() = + member val MyWriteOnlyProperty = 0 with set + +let someValue = + async { + let someInstance = SomeClass() + someInstance.MyWriteOnlyProperty <- 2 + }""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.FavourNonMutablePropertyInitializationShouldNotProduceError_1() = + this.Parse """ +type SomeClass() = + let mutable myInternalValue = 1 + // A write-only property. + member this.MyWriteOnlyProperty with set (value) = myInternalValue <- value + +module Program = + let someFunction() = + let someInstance = SomeClass(MyWriteOnlyProperty = 2) + ()""" + + Assert.IsTrue this.NoErrorsExist + + [] + member this.FavourNonMutablePropertyInitializationShouldNotProduceError_2() = + this.Parse """ +type SomeClass() = + let mutable myInternalValue = 1 + // A read-write property. + member this.MyReadWriteProperty + with get () = myInternalValue + and set (value) = myInternalValue <- value + +module Program = + let someFunction() = + let someInstance = SomeClass(MyReadWriteProperty = 2) + ()""" + + Assert.IsTrue this.NoErrorsExist + + [] + member this.FavourNonMutablePropertyInitializationShouldNotProduceError_3() = + this.Parse """ +type SomeClass() = + let mutable myInternalValue = 1 + member this.SomeMethod() = + () // some code here + // A read-write property. + member this.MyReadWriteProperty + with get () = myInternalValue + and set (value) = myInternalValue <- value + +module Program = + let someFunction = + let someInstance = SomeClass() + someInstance.SomeMethod() + someInstance.MyReadWriteProperty <- 2""" + + Assert.IsTrue this.NoErrorsExist + + [] + member this.FavourNonMutablePropertyInitializationShouldNotProduceError_4() = + this.Parse """ +module SomeModule = + let mutable myInternalValue = 1 + +module Program = + let someFunction() = + SomeModule.myInternalValue <- 2""" + + Assert.IsTrue this.NoErrorsExist + + [] + member this.FavourNonMutablePropertyInitializationShouldNotProduceError_5() = + this.Parse """ +open System.Net + +module Program = + let someFunction() = + // System.Net.Cookie implementation lives in a referenced assembly + let someInstance = Cookie(Domain = "example.com") + ()""" + + Assert.IsTrue this.NoErrorsExist + + [] + member this.``FavourNonMutablePropertyInitialization should not produce error on local variables``() = + this.Parse """ +let someFunc () = + let mutable current = 23 + current <- 2 + ()""" + + Assert.IsTrue this.NoErrorsExist