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.

Sign up for GitHub

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

Error when property has same name as DU case #16760

Closed
wants to merge 10 commits into from
Closed

Error when property has same name as DU case #16760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8241e06
Error when property has same name as DU case
edgarfgp Feb 24, 2024
23df1d9
Merge branch 'main' into fix-16646
edgarfgp Feb 25, 2024
dee4477
Update tests
edgarfgp Feb 25, 2024
af51309
Merge branch 'fix-16646' of github.com:edgarfgp/fsharp into fix-16646
edgarfgp Feb 25, 2024
eb97c47
fix test
edgarfgp Feb 25, 2024
0916405
Merge branch 'main' into fix-16646
edgarfgp Feb 26, 2024
8074f51
Merge branch 'main' into fix-16646
edgarfgp Feb 26, 2024
65a4834
fix test
edgarfgp Feb 25, 2024
ca6a6cc
Merge branch 'fix-16646' of github.com:edgarfgp/fsharp into fix-16646
edgarfgp Feb 26, 2024
0d79645
update condition
edgarfgp Feb 26, 2024
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
11 changes: 10 additions & 1 deletion src/Compiler/Checking/CheckExpressions.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9406,6 +9406,16 @@ and TcLookupItemThen cenv overallTy env tpenv mObjExpr objExpr objExprTy delayed
TcTraitItemThen cenv overallTy env (Some objExpr) traitInfo tpenv mItem delayed

| Item.DelegateCtor _ -> error (Error (FSComp.SR.tcConstructorsCannotBeFirstClassValues(), mItem))

| Item.UnionCase(info, _) ->
let clashingName =
info.TyconRef.MembersOfFSharpTyconSorted |> List.tryFind (fun mem -> mem.DisplayNameCore = info.DisplayNameCore)

match clashingName with
| None -> error (Error (FSComp.SR.tcSyntaxFormUsedOnlyWithRecordLabelsPropertiesAndFields(), mItem))
| Some value ->
let kind = (if value.IsMember then "member" else "value")
error(NameClash(info.DisplayNameCore, kind, info.DisplayNameCore, mItem, FSComp.SR.typeInfoUnionCase(), info.DisplayNameCore, mItem))

// These items are not expected here - they can't be the result of a instance member dot-lookup "expr.Ident"
| Item.ActivePatternResult _
Expand All @@ -9416,7 +9426,6 @@ and TcLookupItemThen cenv overallTy env tpenv mObjExpr objExpr objExprTy delayed
| Item.ModuleOrNamespaces _
| Item.TypeVar _
| Item.Types _
| Item.UnionCase _
| Item.UnionCaseField _
| Item.UnqualifiedType _
| Item.Value _
Expand Down
29 changes: 28 additions & 1 deletion tests/FSharp.Compiler.ComponentTests/Language/DotLambdaTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,4 +290,31 @@ let _ = asQ.Select _.Length
"""
|> withLangVersion80
|> typecheck
|> shouldSucceed
|> shouldSucceed

[<Fact>]
let ``Error when property has same name as DU case`` () =
Fsx """
type MyId =
| IdA of int
| IdB of string

member this.IdA =
match this with
| IdA x -> Some x
| _ -> None

member this.IdX =
match this with
| IdB x -> Some x
| _ -> None

let onlyIdX (ids: MyId list) = ids |> List.choose _.IdX
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@psfinaki This now raises an error message if you accessing to a member that has the same name as the union case. reusing FS0023

(Error 23, Line 17, Col 51, Line 17, Col 56, "The member 'IdA' can not be defined because the name 'IdA' clashes with the union case 'IdA' in this type or module")

I propose we update the error message so it can be used in both cases ie

  • member is defined
  • member is used

Avoiding having to create another error number and message. Maybe

  • The member 'IdA' can not be used because 'IdA' clashes with the union case 'IdA' in this type or module

  • The name 'IdA' can not be used because 'IdA' clashes with the union case 'IdA' in this type or module

  • The member 'IdA' can not be defined or accessed because the name 'IdA' clashes with the union case 'IdA' in this type or module

cc @vzarytovskii

Copy link
Member

Choose a reason for hiding this comment

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

Hmm but why not having both errors here? As in,

type MyId =
    | IdA of int
    | IdB of string

    member this.IdA =
        match this with
        | IdA x -> Some x
        | _ -> None
 
let onlyIdA (ids: MyId list) = ids |> List.choose _.IdA

...would produce both
FS0023 The member 'IdA' can not be defined because the name 'IdA' clashes with the union case 'IdA' in this type or module and
FS0812 The syntax 'expr.id' may only be used with record labels, properties and fields,
as if they were independent (which they kind of are)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that the existing logic uses error(Error(..)) which prevents the analysis to continue so even if we try to add both they would cancel each other. Ideally it would be good to change to use errorR but will require rewriting of pat of the analysis. The best we can do for now is to reuse FS0023

Copy link
Member

Choose a reason for hiding this comment

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

I mean I probably misunderstand something here - why cannot/shouldn't we fix the error recovery in this case instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not that we can't. But it will required updating/splitting the upstream analysis to make it work as error returns exn -> 'T and errorR returns exn -> unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if you feel adventurous we can maybe pair on this, maybe in an AmplifyingF# session :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context: This is the relevant piece of code that will needs updating https://github.com/dotnet/fsharp/blob/f29ce0aef9b70bfef03ef7466ec823e449bb864b/src/Compiler/Checking/CheckExpressions.fs#L9270C5-L9270C21

Copy link
Member

Choose a reason for hiding this comment

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

Well I just believe this can be an opportunity to fix the underlying issue rather than patch the symptome. But that's just my opinion 👀

Otherwise, "The member 'IdA' can not be defined or accessed" is something I would pick out of the options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well at the very least I would give a try and report some progress :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@psfinaki Im revisiting this here #17088

let onlyIdA (ids: MyId list) = ids |> List.choose _.IdA
"""
|> ignoreWarnings
|> compile
|> shouldFail
|> withDiagnostics [
(Error 23, Line 17, Col 51, Line 17, Col 56, "The member 'IdA' can not be defined because the name 'IdA' clashes with the union case 'IdA' in this type or module")
]
Loading