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

Better anonymous record error reporting #15598

Merged
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
b09d8dc
Better anonymous record error reporting
edgarfgp Jul 13, 2023
fec845b
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 13, 2023
229192e
remove extra ticks
edgarfgp Jul 13, 2023
80a3fdb
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 13, 2023
70c34d9
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 19, 2023
216a6e2
Another possible approach
edgarfgp Jul 19, 2023
b075dbb
Remove previous approach and update tests
edgarfgp Jul 19, 2023
d528edc
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 19, 2023
8fa772e
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 19, 2023
9900e78
Don't change unrelated lines
edgarfgp Jul 19, 2023
0aec3c0
Merge branch 'inaccurate-error-in-anonymous-record' of github.com:edg…
edgarfgp Jul 19, 2023
b9213fc
revert unrelated change to make easy to review
edgarfgp Jul 19, 2023
f62fc28
SendEntityPathToSink
edgarfgp Jul 19, 2023
3e9200b
Add struct anon record test
edgarfgp Jul 19, 2023
f65c8c8
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 19, 2023
372c4c1
Fix merge
edgarfgp Jul 19, 2023
78297dc
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 20, 2023
3a629db
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 21, 2023
a13e971
Update error name and message
edgarfgp Jul 21, 2023
73f3a9b
Use the type name as part of the error message
edgarfgp Jul 21, 2023
b1a4037
More tests
edgarfgp Jul 21, 2023
31b8222
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 21, 2023
d26aed5
format code
edgarfgp Jul 21, 2023
adc45f1
Move to CheckExpressions to avoid poluting sig file
edgarfgp Jul 23, 2023
1556f76
Add Symbol tests
edgarfgp Jul 23, 2023
622f56f
Update to minimize diff
edgarfgp Jul 23, 2023
4ef7e80
Update ConvertToAnonymousRecordQuickFix and Tests
edgarfgp Jul 24, 2023
f16ef8e
revert changes to minimize diff
edgarfgp Jul 24, 2023
53a9359
Fix PR comments
edgarfgp Jul 25, 2023
cd92b3c
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 25, 2023
1d111fb
Symbols test take 3
edgarfgp Jul 25, 2023
2237714
Merge branch 'inaccurate-error-in-anonymous-record' of https://github…
edgarfgp Jul 25, 2023
6e8e08b
take 4
edgarfgp Jul 25, 2023
ccefaed
take 5
edgarfgp Jul 25, 2023
32f159a
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 26, 2023
29f757b
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 27, 2023
9480d75
Merge branch 'main' into inaccurate-error-in-anonymous-record
edgarfgp Jul 27, 2023
f5f27af
Ok one more time
edgarfgp Jul 27, 2023
468396f
Merge branch 'inaccurate-error-in-anonymous-record' of https://github…
edgarfgp Jul 27, 2023
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
26 changes: 21 additions & 5 deletions src/Compiler/Checking/CheckExpressions.fs
Original file line number Diff line number Diff line change
Expand Up @@ -1796,12 +1796,20 @@ let FreshenAbstractSlot g amap m synTyparDecls absMethInfo =
let retTyFromAbsSlot = retTy |> GetFSharpViewOfReturnType g |> instType typarInstFromAbsSlot
typarsFromAbsSlotAreRigid, typarsFromAbsSlot, argTysFromAbsSlot, retTyFromAbsSlot

let private CheckCopyUpdateSyntaxInAnonRecords sink (g: TcGlobals) ty (tyIdent: Ident option) (fldId: Ident) nenv ad =
match TryFindAnonRecdFieldOfType g ty fldId.idText, tyIdent with
| Some item, Some tpId ->
CallNameResolutionSink sink (fldId.idRange, nenv, item, emptyTyparInst, ItemOccurence.UseInType, ad)
error(Error(FSComp.SR.chkCopyUpdateSyntaxInAnonRecords(item.DisplayNameCore, tpId.idText, fldId.idText), fldId.idRange))
| _, _ ->
error(UndefinedName(0, FSComp.SR.undefinedNameRecordLabel, fldId, NoSuggestions))

//-------------------------------------------------------------------------
// Helpers to typecheck expressions and patterns
//-------------------------------------------------------------------------

/// Helper used to check record expressions and record patterns
let BuildFieldMap (cenv: cenv) env isPartial ty (flds: ((Ident list * Ident) * 'T) list) m =
let BuildFieldMap (cenv: cenv) env isPartial ty (tyIdent: Ident option) (flds: ((Ident list * Ident) * 'T) list) m =
let g = cenv.g
let ad = env.eAccessRights

Expand All @@ -1813,10 +1821,14 @@ let BuildFieldMap (cenv: cenv) env isPartial ty (flds: ((Ident list * Ident) * '
let allFields = flds |> List.map (fun ((_, ident), _) -> ident)
flds
|> List.choose (fun (fld, fldExpr) ->
let fldPath, fldId = fld
try
let fldPath, fldId = fld
let frefSet = ResolveField cenv.tcSink cenv.nameResolver env.eNameResEnv ad ty fldPath fldId allFields
Some(fld, frefSet, fldExpr)
if isAnonRecdTy cenv.g ty || isStructAnonRecdTy cenv.g ty then
CheckCopyUpdateSyntaxInAnonRecords cenv.tcSink cenv.g ty tyIdent fldId env.eNameResEnv ad
None
else
let frefSet = ResolveField cenv.tcSink cenv.nameResolver env.eNameResEnv ad ty fldPath fldId allFields
Some(fld, frefSet, fldExpr)
with e ->
errorRecoveryNoRange e
None
Expand Down Expand Up @@ -7388,7 +7400,11 @@ and TcRecdExpr cenv overallTy env tpenv (inherits, withExprOpt, synRecdFields, m
match flds with
| [] -> []
| _ ->
match BuildFieldMap cenv env hasOrigExpr overallTy flds mWholeExpr with
let tyIdent =
match withExprOpt with
| Some (SynExpr.Ident ident, _) -> Some ident
edgarfgp marked this conversation as resolved.
Show resolved Hide resolved
| _ -> None
match BuildFieldMap cenv env hasOrigExpr overallTy tyIdent flds mWholeExpr with
| None -> []
| Some(tinst, tcref, _, fldsList) ->

Expand Down
1 change: 1 addition & 0 deletions src/Compiler/Checking/CheckExpressions.fsi
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,7 @@ val BuildFieldMap:
env: TcEnv ->
isPartial: bool ->
ty: TType ->
tyIdent: Ident option ->
flds: ((Ident list * Ident) * 'T) list ->
m: range ->
(TypeInst * TyconRef * Map<string, 'T> * (string * 'T) list) option
Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/Checking/CheckPatterns.fs
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ and TcPatArrayOrList warnOnUpper cenv env vFlags patEnv ty isArray args m =

and TcRecordPat warnOnUpper cenv env vFlags patEnv ty fieldPats m =
let fieldPats = fieldPats |> List.map (fun (fieldId, _, fieldPat) -> fieldId, fieldPat)
match BuildFieldMap cenv env true ty fieldPats m with
match BuildFieldMap cenv env true ty None fieldPats m with
| None -> (fun _ -> TPat_error m), patEnv
| Some(tinst, tcref, fldsmap, _fldsList) ->

Expand Down
3 changes: 1 addition & 2 deletions src/Compiler/Checking/NameResolution.fs
Original file line number Diff line number Diff line change
Expand Up @@ -3716,8 +3716,7 @@ let ResolveFieldPrim sink (ncenv: NameResolver) nenv ad ty (mp, id: Ident) allFi
error(ErrorWithSuggestions(errorText, m, id.idText, suggestLabels))
else
lookup()
| _ ->
lookup()
| ValueNone -> lookup()
| _ ->
let lid = (mp@[id])
let tyconSearch ad () =
Expand Down
3 changes: 2 additions & 1 deletion src/Compiler/FSComp.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1708,4 +1708,5 @@ featureAccessorFunctionShorthand,"underscore dot shorthand for accessor only fun
3569,chkNotTailRecursive,"The member or function '%s' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way."
3570,tcStaticBindingInExtrinsicAugmentation,"Static bindings cannot be added to extrinsic augmentations. Consider using a 'static member' instead."
3571,pickleFsharpCoreBackwardsCompatible,"Newly added pickle state cannot be used in FSharp.Core, since it must be working in older compilers+tooling as well. The time window is at least 3 years after feature introduction. Violation: %s . Context: \n %s "
3577,tcOverrideUsesMultipleArgumentsInsteadOfTuple,"This override takes a tuple instead of multiple arguments. Try to add an additional layer of parentheses at the method definition (e.g. 'member _.Foo((x, y))'), or remove parentheses at the abstract method declaration (e.g. 'abstract member Foo: 'a * 'b -> 'c')."
3577,tcOverrideUsesMultipleArgumentsInsteadOfTuple,"This override takes a tuple instead of multiple arguments. Try to add an additional layer of parentheses at the method definition (e.g. 'member _.Foo((x, y))'), or remove parentheses at the abstract method declaration (e.g. 'abstract member Foo: 'a * 'b -> 'c')."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
3578,chkCopyUpdateSyntaxInAnonRecords,"Label '%s' is part of anonymous record. Use {{| %s with %s = ... |}} instead."
3578,chkCopyUpdateSyntaxInAnonRecords,"Value '%s' is an anonymous record. Use syntax {{| %s with %s = ... |}} to update it."
  • switch the arguments.
    Imo the name of the anon record value should be the subject in this sentence.

Copy link
Member

@vzarytovskii vzarytovskii Jul 24, 2023

Choose a reason for hiding this comment

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

@T-Gro, probably "use the following syntax ... to update it"? My english is not that good, maybe @KathleenDollard can help us here?

Copy link
Contributor Author

@edgarfgp edgarfgp Jul 24, 2023

Choose a reason for hiding this comment

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

Happy to adjust the error message. I was hoping for an error message to be suggested :).

Copy link
Member

Choose a reason for hiding this comment

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

Do we actually use term Label for record fields anywhere in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@auduchinok I will wait to see what is the final suggested message to update it :)

Copy link
Contributor Author

@edgarfgp edgarfgp Jul 27, 2023

Choose a reason for hiding this comment

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

In my initial implementation, I had the Ident name from expr but as Eugene pointed out here. I was only checking for one specific shape of the SynExpr and that will eliminate the error reporting for the rest of the cases where an anonymous record value can be obtained like from a property (x.Prop), a function call (f ()), an indexer (x.[0], or x[0]), etc.

I think are more general approach needs to be used here so that is why I'm using expr

Update: I'm happy to change the error message(based on not using the Ident name) to your suggestion if that is work for the team

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we can say This expression is an anonymous record, use {|...|} instead of {...}. if the whole { ... } is underlined. I would prefer that over mentioning a particular field from the record. What do others think?

Copy link
Contributor Author

@edgarfgp edgarfgp Jul 27, 2023

Choose a reason for hiding this comment

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

This PR uses a more accurate error range based on the annon record field that is being used.

Edit: Also this is what the issue that this PR tries to solve mentions #14814

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't seem very accurate to me, seeing that the fields have nothing to do with the error.

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 mean they are related as it is trying to update an annon record using the record syntax. and we now that because 2 of the fields belong to an annon record. ?

3578,chkCopyUpdateSyntaxInAnonRecords,"Label '%s' is part of anonymous record. Use {{| %s with %s = ... |}} instead."
5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.it.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.ja.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.ko.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.pl.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.pt-BR.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.ru.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.tr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.zh-Hans.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compiler/xlf/FSComp.txt.zh-Hant.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading