Skip to content

Commit 0ae7c7b

Browse files
knocteknocte
authored
Merge PR #683 from webwarrior-ws/favourNonMutablePropertyInitialization-rebased
* Add FavourNonMutablePropertyInitialization rule and tests for it. * Apply suggestions of this rule to FSharpLint code. Fixes #535
2 parents 0d0a308 + 9e5ed77 commit 0ae7c7b

File tree

11 files changed

+411
-15
lines changed

11 files changed

+411
-15
lines changed

‎docs/content/how-tos/rule-configuration.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,3 +124,4 @@ The following rules can be specified for linting.
124124
- [NestedFunctionNames (FL0081)](rules/FL0081.html)
125125
- [UsedUnderscorePrefixedElements (FL0082)](rules/FL0082.html)
126126
- [UnneededRecKeyword (FL0083)](rules/FL0083.html)
127+
- [FavourNonMutablePropertyInitialization (FL0084)](rules/FL0084.html)

‎docs/content/how-tos/rules/FL0084.md‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
---
2+
title: FL0084
3+
category: how-to
4+
hide_menu: true
5+
---
6+
7+
# FavourNonMutablePropertyInitialization (FL0084)
8+
9+
*Introduced in `0.24.0`*
10+
11+
## Cause
12+
13+
Use of mutation to initialize properties with values.
14+
15+
## Rationale
16+
17+
There's no need to use mutation to initialize properties with values because F# has specific syntax for this kind of intitialization.
18+
Both approaches will compile to the same IL but non-mutable code doesn't look unsafe because it prevents the use of the `<-` operator.
19+
20+
## How To Fix
21+
22+
Replace all occurrences of mutable property initialization with non mutable property initialization.
23+
Example: `Cookie(Domain = "example.com")` instead of `let c = Cookie(); c.Domain <- "example.com"`
24+
25+
## Rule Settings
26+
27+
{
28+
"FavourNonMutablePropertyInitialization": { "enabled": false }
29+
}

‎src/FSharpLint.Core/Application/Configuration.fs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,7 @@ module FSharpJsonConverter =
4848
|]
4949

5050
let serializerSettings =
51-
let settings = JsonSerializerSettings()
52-
settings.NullValueHandling <- NullValueHandling.Ignore
53-
settings.Converters <- converters
54-
settings
51+
JsonSerializerSettings(NullValueHandling = NullValueHandling.Ignore, Converters = converters)
5552

5653
module IgnoreFiles =
5754

@@ -318,6 +315,7 @@ type ConventionsConfig =
318315
favourStaticEmptyFields:EnabledConfig option
319316
asyncExceptionWithoutReturn:EnabledConfig option
320317
unneededRecKeyword:EnabledConfig option
318+
favourNonMutablePropertyInitialization:EnabledConfig option
321319
nestedStatements:RuleConfig<NestedStatements.Config> option
322320
cyclomaticComplexity:RuleConfig<CyclomaticComplexity.Config> option
323321
reimplementsFunction:EnabledConfig option
@@ -338,6 +336,7 @@ with
338336
this.recursiveAsyncFunction |> Option.bind (constructRuleIfEnabled RecursiveAsyncFunction.rule) |> Option.toArray
339337
this.avoidTooShortNames |> Option.bind (constructRuleIfEnabled AvoidTooShortNames.rule) |> Option.toArray
340338
this.redundantNewKeyword |> Option.bind (constructRuleIfEnabled RedundantNewKeyword.rule) |> Option.toArray
339+
this.favourNonMutablePropertyInitialization |> Option.bind (constructRuleIfEnabled FavourNonMutablePropertyInitialization.rule) |> Option.toArray
341340
this.favourReRaise |> Option.bind (constructRuleIfEnabled FavourReRaise.rule) |> Option.toArray
342341
this.favourStaticEmptyFields |> Option.bind (constructRuleIfEnabled FavourStaticEmptyFields.rule) |> Option.toArray
343342
this.asyncExceptionWithoutReturn |> Option.bind (constructRuleIfEnabled AsyncExceptionWithoutReturn.rule) |> Option.toArray
@@ -412,6 +411,7 @@ type Configuration =
412411
RecursiveAsyncFunction:EnabledConfig option
413412
AvoidTooShortNames:EnabledConfig option
414413
RedundantNewKeyword:EnabledConfig option
414+
FavourNonMutablePropertyInitialization:EnabledConfig option
415415
FavourReRaise:EnabledConfig option
416416
FavourStaticEmptyFields:EnabledConfig option
417417
AsyncExceptionWithoutReturn:EnabledConfig option
@@ -501,6 +501,7 @@ with
501501
RecursiveAsyncFunction = None
502502
AvoidTooShortNames = None
503503
RedundantNewKeyword = None
504+
FavourNonMutablePropertyInitialization = None
504505
FavourReRaise = None
505506
FavourStaticEmptyFields = None
506507
AsyncExceptionWithoutReturn = None
@@ -653,6 +654,7 @@ let flattenConfig (config:Configuration) =
653654
config.RecursiveAsyncFunction |> Option.bind (constructRuleIfEnabled RecursiveAsyncFunction.rule)
654655
config.AvoidTooShortNames |> Option.bind (constructRuleIfEnabled AvoidTooShortNames.rule)
655656
config.RedundantNewKeyword |> Option.bind (constructRuleIfEnabled RedundantNewKeyword.rule)
657+
config.FavourNonMutablePropertyInitialization |> Option.bind (constructRuleIfEnabled FavourNonMutablePropertyInitialization.rule)
656658
config.FavourReRaise |> Option.bind (constructRuleIfEnabled FavourReRaise.rule)
657659
config.FavourStaticEmptyFields |> Option.bind (constructRuleIfEnabled FavourStaticEmptyFields.rule)
658660
config.AsyncExceptionWithoutReturn |> Option.bind (constructRuleIfEnabled AsyncExceptionWithoutReturn.rule)

‎src/FSharpLint.Core/Application/Lint.fs‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -261,17 +261,18 @@ module Lint =
261261
ReachedEnd(fileInfo.File, fileWarnings |> Seq.toList) |> lintInfo.ReportLinterProgress
262262

263263
let private runProcess (workingDir:string) (exePath:string) (args:string) =
264-
let psi = System.Diagnostics.ProcessStartInfo()
265-
psi.FileName <- exePath
266-
psi.WorkingDirectory <- workingDir
267-
psi.RedirectStandardOutput <- true
268-
psi.RedirectStandardError <- true
269-
psi.Arguments <- args
270-
psi.CreateNoWindow <- true
271-
psi.UseShellExecute <- false
272-
273-
use p = new System.Diagnostics.Process()
274-
p.StartInfo <- psi
264+
let psi =
265+
System.Diagnostics.ProcessStartInfo(
266+
FileName = exePath,
267+
WorkingDirectory = workingDir,
268+
RedirectStandardOutput = true,
269+
RedirectStandardError = true,
270+
Arguments = args,
271+
CreateNoWindow = true,
272+
UseShellExecute = false
273+
)
274+
275+
use p = new System.Diagnostics.Process(StartInfo = psi)
275276
let sbOut = System.Text.StringBuilder()
276277
p.OutputDataReceived.Add(fun ea -> sbOut.AppendLine(ea.Data) |> ignore)
277278
let sbErr = System.Text.StringBuilder()

‎src/FSharpLint.Core/FSharpLint.Core.fsproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@
5555
<Compile Include="Rules\Conventions\AvoidSinglePipeOperator.fs" />
5656
<Compile Include="Rules\Conventions\SuggestUseAutoProperty.fs" />
5757
<Compile Include="Rules\Conventions\UsedUnderscorePrefixedElements.fs" />
58+
<Compile Include="Rules\Conventions\FavourNonMutablePropertyInitialization.fs" />
5859
<Compile Include="Rules\Conventions\RaiseWithTooManyArguments\RaiseWithTooManyArgumentsHelper.fs" />
5960
<Compile Include="Rules\Conventions\RaiseWithTooManyArguments\FailwithWithSingleArgument.fs" />
6061
<Compile Include="Rules\Conventions\RaiseWithTooManyArguments\RaiseWithSingleArgument.fs" />

‎src/FSharpLint.Core/Rules/Conventions/FavourNonMutablePropertyInitialization.fs‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
module FSharpLint.Rules.FavourNonMutablePropertyInitialization
2+
3+
open FSharpLint.Framework
4+
open FSharpLint.Framework.Suggestion
5+
open FSharp.Compiler.Syntax
6+
open FSharpLint.Framework.Ast
7+
open FSharpLint.Framework.Rules
8+
open System
9+
10+
let private getWarningDetails (ident: Ident) =
11+
let formatError errorName =
12+
String.Format(Resources.GetString errorName, ident.idText)
13+
14+
"RulesFavourNonMutablePropertyInitializationError"
15+
|> formatError
16+
|> Array.singleton
17+
|> Array.map (fun message ->
18+
{ Range = ident.idRange
19+
Message = message
20+
SuggestedFix = None
21+
TypeChecks = List.Empty })
22+
23+
let private extraInstanceMethod (app:SynExpr) (instanceMethodCalls: List<string>) =
24+
match app with
25+
| SynExpr.App(_, _, expression, _, _) ->
26+
match expression with
27+
| SynExpr.LongIdent(_, SynLongIdent(identifiers, _, _), _, _) ->
28+
match List.tryLast identifiers with
29+
| Some _ ->
30+
identifiers.[0].idText::instanceMethodCalls
31+
| _ -> instanceMethodCalls
32+
| _ -> instanceMethodCalls
33+
| _ -> instanceMethodCalls
34+
35+
let rec private extraFromBindings (bindings: List<SynBinding>) (classInstances: List<string>) =
36+
match bindings with
37+
| SynBinding(_, _, _, _, _, _, _, SynPat.Named(SynIdent(ident, _), _, _, _), _, _expression, _, _, _)::rest ->
38+
extraFromBindings rest (ident.idText::classInstances)
39+
| _ -> classInstances
40+
41+
let rec private processLetBinding (instanceNames: Set<string>) (body: SynExpr) : array<WarningDetails> =
42+
match body with
43+
| SynExpr.LongIdentSet(SynLongIdent(identifiers, _, _), _, _) ->
44+
match identifiers with
45+
| [instanceIdent; propertyIdent] when Set.contains instanceIdent.idText instanceNames ->
46+
getWarningDetails propertyIdent
47+
| _ -> Array.empty
48+
| SynExpr.Sequential(_, _, expr1, expr2, _) ->
49+
let instanceNames =
50+
Set.difference
51+
instanceNames
52+
(extraInstanceMethod expr1 List.empty |> Set.ofList)
53+
Array.append
54+
(processLetBinding instanceNames expr1)
55+
(processLetBinding instanceNames expr2)
56+
| _ -> [||]
57+
58+
and processExpression (expression: SynExpr) : array<WarningDetails> =
59+
match expression with
60+
| SynExpr.LetOrUse(_, _, bindings, body, _, _) ->
61+
let instanceNames = extraFromBindings bindings [] |> Set.ofList
62+
processLetBinding instanceNames body
63+
| SynExpr.Sequential(_, _, expr1, expr2, _) ->
64+
Array.append
65+
(processExpression expr1)
66+
(processExpression expr2)
67+
| _ -> Array.empty
68+
69+
let runner args =
70+
match args.AstNode with
71+
| Binding(SynBinding(_, _, _, _, _, _, _, _, _, SynExpr.LetOrUse(_, _, bindings, body, _, _), _, _, _)) ->
72+
let instanceNames = extraFromBindings bindings [] |> Set.ofList
73+
processLetBinding instanceNames body
74+
| Match(SynMatchClause(_, _, expr, _, _, _)) ->
75+
processExpression expr
76+
| Lambda(lambda, _) ->
77+
processExpression lambda.Body
78+
| Expression(SynExpr.TryWith(tryExpr, _, _, _, _, _)) ->
79+
processExpression tryExpr
80+
| Expression(SynExpr.TryFinally(tryExpr, finallyExpr, _, _, _, _)) ->
81+
Array.append
82+
(processExpression tryExpr)
83+
(processExpression finallyExpr)
84+
| Expression(SynExpr.ComputationExpr(_, expr, _)) ->
85+
processExpression expr
86+
| _ -> Array.empty
87+
88+
let rule =
89+
{ Name = "FavourNonMutablePropertyInitialization"
90+
Identifier = Identifiers.FavourNonMutablePropertyInitialization
91+
RuleConfig = { AstNodeRuleConfig.Runner = runner; Cleanup = ignore } }
92+
|> AstNodeRule

‎src/FSharpLint.Core/Rules/Identifiers.fs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,3 +88,4 @@ let UnnestedFunctionNames = identifier 80
8888
let NestedFunctionNames = identifier 81
8989
let UsedUnderscorePrefixedElements = identifier 82
9090
let UnneededRecKeyword = identifier 83
91+
let FavourNonMutablePropertyInitialization = identifier 84

‎src/FSharpLint.Core/Text.resx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,4 +363,7 @@
363363
<data name="RulesUnneededRecKeyword" xml:space="preserve">
364364
<value>The '{0}' function has a "rec" keyword, but it is not really recursive, consider removing it.</value>
365365
</data>
366+
<data name="RulesFavourNonMutablePropertyInitializationError" xml:space="preserve">
367+
<value>Consider using non-mutable property initialization.</value>
368+
</data>
366369
</root>

‎src/FSharpLint.Core/fsharplint.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,7 @@
286286
"uselessBinding": { "enabled": true },
287287
"tupleOfWildcards": { "enabled": true },
288288
"favourTypedIgnore": { "enabled": false },
289+
"favourNonMutablePropertyInitialization": { "enabled": true },
289290
"favourReRaise": { "enabled": true },
290291
"favourStaticEmptyFields": { "enabled": false },
291292
"favourConsistentThis": {

‎tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
<Compile Include="Rules\Conventions\AvoidSinglePipeOperator.fs" />
4545
<Compile Include="Rules\Conventions\SuggestUseAutoProperty.fs" />
4646
<Compile Include="Rules\Conventions\UsedUnderscorePrefixedElements.fs" />
47+
<Compile Include="Rules\Conventions\FavourNonMutablePropertyInitialization.fs" />
4748
<Compile Include="Rules\Conventions\Naming\NamingHelpers.fs" />
4849
<Compile Include="Rules\Conventions\Naming\InterfaceNames.fs" />
4950
<Compile Include="Rules\Conventions\Naming\ExceptionNames.fs" />

0 commit comments

Comments
 (0)