Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a compiler warning for lowercase literals in patterns #666

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/fsharp/FSComp.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1344,3 +1344,4 @@ estApplyStaticArgumentsForMethodNotImplemented,"A type provider implemented GetS
3187,checkNotSufficientlyGenericBecauseOfScope,"Type inference caused the type variable %s to escape its scope. Consider adding an explicit type parameter declaration or adjusting your code to be less generic."
3188,checkNotSufficientlyGenericBecauseOfScopeAnon,"Type inference caused an inference type variable to escape its scope. Consider adding type annotations to make your code less generic."
3189,checkRaiseFamilyFunctionArgumentCount,"Redundant arguments are being ignored in function '%s'. Expected %d but got %d arguments."
3190,checkLowercaseLiteralBindingInPattern,"Lowercase literal '%s' is being shadowed by a new pattern with the same name. Only uppercase and module-prefixed literals can be used as named patterns."
5 changes: 4 additions & 1 deletion src/fsharp/NameResolution.fs
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,10 @@ let private AddPartsOfTyconRefToNameEnv bulkAddMode ownDefinition (g:TcGlobals)
ePatItems = ePatItems
eIndexedExtensionMembers = eIndexedExtensionMembers
eUnindexedExtensionMembers = eUnindexedExtensionMembers }


let TryFindPatternByName name {ePatItems = patternMap} =
NameMap.tryFind name patternMap

/// Add a set of type definitions to the name resolution environment
let AddTyconRefsToNameEnv bulkAddMode ownDefinition g amap m root nenv tcrefs =
let env = List.fold (AddPartsOfTyconRefToNameEnv bulkAddMode ownDefinition g amap m) nenv tcrefs
Expand Down
3 changes: 3 additions & 0 deletions src/fsharp/NameResolution.fsi
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ type FullyQualifiedFlag =
[<RequireQualifiedAccess>]
type BulkAdd = Yes | No

/// Lookup patterns in name resolution environment
val internal TryFindPatternByName : string -> NameResolutionEnv -> Item option

/// Add extra items to the environment for Visual Studio, e.g. static members
val internal AddFakeNamedValRefToNameEnv : string -> NameResolutionEnv -> ValRef -> NameResolutionEnv

Expand Down
11 changes: 9 additions & 2 deletions src/fsharp/TypeChecker.fs
Original file line number Diff line number Diff line change
Expand Up @@ -4691,7 +4691,7 @@ and TcSimplePatsOfUnknownType cenv optArgsOK checkCxs env tpenv spats =
let argty = NewInferenceType ()
TcSimplePats cenv optArgsOK checkCxs argty env (tpenv,NameMap.empty,Set.empty) spats

and TcPatBindingName _cenv _env id ty isMemberThis vis1 topValData (inlineFlag,declaredTypars,argAttribs,isMutable,vis2,compgen) (names,takenNames:Set<string>) =
and TcPatBindingName _cenv env id ty isMemberThis vis1 topValData (inlineFlag,declaredTypars,argAttribs,isMutable,vis2,compgen) (names,takenNames:Set<string>) =
let vis = if isSome vis1 then vis1 else vis2
if takenNames.Contains id.idText then errorR (VarBoundTwice id)
let baseOrThis = if isMemberThis then MemberThisVal else NormalVal
Expand All @@ -4700,7 +4700,14 @@ and TcPatBindingName _cenv _env id ty isMemberThis vis1 topValData (inlineFlag,d
(fun (TcPatPhase2Input values) ->
let (vspec,typeScheme) =
match values.TryFind id.idText with
| Some x -> x
| Some value ->
let name = id.idText
if not (String.IsNullOrEmpty name) && Char.IsLower(name.[0]) then
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when would name be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it will happen. The check is there to make sure that the index access (name.[0]) is totally safe.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok me neither. However it does seem surprising that a identifier is the empty string.

match TryFindPatternByName name env.eNameResEnv with
| Some (Item.Value vref) when vref.LiteralValue.IsSome ->
warning(Error(FSComp.SR.checkLowercaseLiteralBindingInPattern(id.idText),id.idRange))
| Some _ | None -> ()
value
| None -> error(Error(FSComp.SR.tcNameNotBoundInPattern(id.idText),id.idRange))
PBind(vspec,typeScheme)),
names,takenNames
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// #Regression #Diagnostics
//<Expects status="warning" span="(10,5-10,14)" id="FS3190">Lowercase literal 'lowerCase' is being shadowed by a new pattern with the same name\. Only uppercase and module-prefixed literals can be used as named patterns\.$</Expects>
module M

let [<Literal>] lowerCase = "lowerCase"
let [<Literal>] UpperCase = "UpperCase"

let f = function
| UpperCase -> "UpperCase"
| lowerCase -> "LowerCase"

f "A" |> ignore

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// #Regression #Diagnostics
//<Expects status="warning" span="(13,7-13,8)" id="FS0026">This rule will never be matched$</Expects>
module M0

module m1 =
let [<Literal>] lowerCase = "lowerCase"
let [<Literal>] UpperCase = "UpperCase"

module M2 =
let f = function
| m1.lowerCase -> "LowerCase"
| lowerCase2 -> "LowerCase2"
| _ -> "Don't know"

printfn "%A" (M2.f "B")
5 changes: 4 additions & 1 deletion tests/fsharpqa/Source/Diagnostics/General/env.lst
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,7 @@ ReqPP SOURCE=W_WebExtensionsNotInPowerPack01.fs SCFLAGS="--test:ErrorRanges -r:F
SOURCE=W_RaiseRedundantArgs.fs SCFLAGS="--test:ErrorRanges -a" # W_RaiseRedundantArgs.fs
SOURCE=W_InvalidArgRedundantArgs.fs SCFLAGS="--test:ErrorRanges -a" # W_InvalidArgRedundantArgs.fs
SOURCE=W_NullArgRedundantArgs.fs SCFLAGS="--test:ErrorRanges -a" # W_NullArgRedundantArgs.fs
SOURCE=W_InvalidOpRedundantArgs.fs SCFLAGS="--test:ErrorRanges -a" # W_InvalidOpRedundantArgs.fs
SOURCE=W_InvalidOpRedundantArgs.fs SCFLAGS="--test:ErrorRanges -a" # W_InvalidOpRedundantArgs.fs

SOURCE=W_LowercaseLiteralIgnored.fs SCFLAGS="--test:ErrorRanges" # W_LowercaseLiteralIgnored.fs
SOURCE=W_LowercaseLiteralNotIgnored.fs SCFLAGS="--test:ErrorRanges" # W_LowercaseLiteralNotIgnored.fs