Fix for #83 (improve constraint error message) by Martin521 · Pull Request #16304 · dotnet/fsharp

Martin521 · 2023-11-19T14:58:30Z

This fix for #83 includes

An addition of field "source" to TraitConstraintInfo / TTrait in TypedTree.fs/fsi
Code in CompilerImports.fs which, after import, adds the source of the constraint to all constraints
A new error message in FSComp.txt
Application of the error message, including the constraint source, in ConstraintSolver.fs
4 new tests in MemberConstraints.fs
A few updated test expectations in AbsTests.fs, neg61.bsl and E_NoMethodsOnEnums01.fs
29 adaptations in 9 files because of the new field in TTrait

This includes - An addition of field "source" to TraitConstraintInfo / TTrait in TypedTree.fs/fsi - Code in CompilerImports.fs which, after import, adds the sources to all constraints - New error messages, including the constraint source, in ConstraintSolver.fs - The error messages in FSComp.txt - A new test in MemberConstraints.fs - 29 adaptations b.o. the new field in TTrait in 9 files

Martin521 · 2023-11-19T16:57:05Z

/run xlf

Co-authored-by: Martin521 <29605222+Martin521@users.noreply.github.com>

github-actions · 2023-11-19T16:57:53Z

Ran xlf: https://github.com/dotnet/fsharp/actions/runs/6921785319

psfinaki · 2023-11-20T17:52:28Z

Hey @Martin521 - thanks for taking this long-standing issue! :)

I know there is a discussion in the related ticket - my opinion is that whatever we do will be better than the current situation. That said, for me even having DivideByInt notion in the error message is weird, feels like a leaking technical detail. The code in the new test can very well be written by some F# newcomer and that message would just explode their mind. I would prefer something like "Hey, the average function expects int result which cannot be guaranteed by division".

I see that the message in FSComp is generic for types and operators so for now I would appreciate just adding a few more test cases that are expected to produce the same error, so that we can see a bigger picture.

Martin521 · 2023-11-20T18:54:39Z

@psfinaki - thanks for your feedback!

I see that the message in FSComp is generic for types and operators so for now I would appreciate just adding a few more test cases that are expected to produce the same error, so that we can see a bigger picture.

I will do that.

I will also respond (tomorrow) to your other remarks, but in the ticket.

smoothdeveloper · 2023-11-20T19:06:15Z

@psfinaki,

the average function expects int result which cannot be guaranteed by division

AFAIU, this would require infrastructure for API authors, and adjusting FSharp.Core to use it; the infrastructure would allow to give customized error message on members with constraints, given predicate on the passed / infered type arguments, to deliver a custom message.

IMO, this is out of scope in just trying to improve the constraint not matching error, which should, unless the infrastructure & special condition that it would detect in API, always be displayed, in order to give the full picture.

vzarytovskii · 2023-11-20T19:19:37Z

That said, for me even having DivideByInt notion in the error message is weird, feels like a leaking technical detail. The code in the new test can very well be written by some F# newcomer and that message would just explode their mind. I would prefer something like "Hey, the average function expects int result which cannot be guaranteed by division".

I personally disagree with that. It doesn't have to be either/or. It should be both - a simple message as well as bunch of details.

We don't need to simplify messages in sake of simplifying them. But rather extend them.

@DedSec256 and @auduchinok may have some opinions here.

psfinaki · 2023-11-20T20:41:47Z

No yeah for sure we shouldn't overhaul the compiler just in order to make this usecase clear.

We don't need to simplify messages in sake of simplifying them. But rather extend them.

... is definitely a valid approach.

Martin521 · 2023-11-21T15:06:19Z

Thanks @psfinaki for asking for more examples and the broader picture. This made me realize that the picture is broader indeed. I will continue with implementation (including test cases) once the discussion on the ticket is converging.

since the second error it is introducing is not related to methods on enums and is difficult to verify.

Martin521 · 2023-12-12T13:29:53Z

This is ready.
This fixes #83 for the limited scope mentioned here.

Also note that the extended error message only appears when the constraint source (the operator or method or function that has the constraint typar) is imported. Doing the same while that source is in your own project is more difficult, but also deemed to be not needed, because both the language guide and the spec say that creating member constraints is "not intended for common use".

Martin521 · 2023-12-18T18:55:20Z

/azp run

azure-pipelines · 2023-12-18T18:55:27Z

Commenter does not have sufficient privileges for PR 16304 in repo dotnet/fsharp

T-Gro · 2023-12-19T15:26:54Z

/azp run

azure-pipelines · 2023-12-19T15:27:10Z

Azure Pipelines successfully started running 2 pipeline(s).

vzarytovskii · 2023-12-20T00:00:37Z

Thanks, let's see how it looks in IDEs, and get feedback from people.

`addConstraintSources` (added in dotnet#16304, so that a failed member constraint names the member it came from) is applied to every imported assembly, and recurses through `e.ModuleOrNamespaceType` for every module and namespace entity it finds. For an assembly imported from IL there is nothing to find: the walk only reads `AllValsAndMembers`, and `ImportILTypeDefs` gives every namespace and type entity an empty val list; only an F# trait constraint produces a `TyparConstraint.MayResolveMember` to label in the first place. Meanwhile the recursion forces each namespace entity's `ModuleOrNamespaceType`, which imports that namespace - so referencing an assembly ends up importing every namespace in it, and reading every type definition, whether or not the code touches it. Skip the CCUs that aren't F#. FSharp.Core and F# references are still walked, so the error messages are unchanged. Measured with FSharpChecker.ParseAndCheckProject, keeping the results alive so the imported assembly structures stay on the heap (averages of 3 runs, one per process): a 486-reference F# project retains 1319.2 -> 952.1 MB (-27.8%), and a 168-reference console project 77.7 -> 69.8 MB (-10.2%). Checking FSharp.Compiler.Service itself (124 references, 397 sources) goes 2301.7 -> 2298.0 MB, i.e. within the noise at that size - what the imports cost there is dwarfed by the trees of the project's own code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…sources (#20090) * Don't walk non-F# assemblies when labelling trait constraint sources `addConstraintSources` (added in #16304, so that a failed member constraint names the member it came from) is applied to every imported assembly, and recurses through `e.ModuleOrNamespaceType` for every module and namespace entity it finds. For an assembly imported from IL there is nothing to find: the walk only reads `AllValsAndMembers`, and `ImportILTypeDefs` gives every namespace and type entity an empty val list; only an F# trait constraint produces a `TyparConstraint.MayResolveMember` to label in the first place. Meanwhile the recursion forces each namespace entity's `ModuleOrNamespaceType`, which imports that namespace - so referencing an assembly ends up importing every namespace in it, and reading every type definition, whether or not the code touches it. Skip the CCUs that aren't F#. FSharp.Core and F# references are still walked, so the error messages are unchanged. Measured with FSharpChecker.ParseAndCheckProject, keeping the results alive so the imported assembly structures stay on the heap (averages of 3 runs, one per process): a 486-reference F# project retains 1319.2 -> 952.1 MB (-27.8%), and a 168-reference console project 77.7 -> 69.8 MB (-10.2%). Checking FSharp.Compiler.Service itself (124 references, 397 sources) goes 2301.7 -> 2298.0 MB, i.e. within the noise at that size - what the imports cost there is dwarfed by the trees of the project's own code.

Martin521 requested a review from a team as a code owner November 19, 2023 14:58

Automated command ran: xlf
f1d5577
Co-authored-by: Martin521 <29605222+Martin521@users.noreply.github.com>

fixed formatting
ba19c54

Martin521 marked this pull request as draft November 19, 2023 19:07

Martin521 added 4 commits November 19, 2023 20:09

fixed regression
e668683

extended test for self-referring constraints
8825860

fix for sign tests
1a4c3a5

adjusted fsharpqa test
e462a17

pbachmann mentioned this pull request Nov 24, 2023
Improve error message for: "The type 'int' does not support the operator 'DivideByInt'" #83
Closed

Martin521 added 12 commits December 10, 2023 18:12

fix looks good to me; all local tests passing
eaefa0c

internal merge
eaf2c6f

changed message text, added correct operator name
05107ab

Merge branch 'main' into issue83fix
2ae2462

change message text, fix logic, add correct operator name
e3f46dc

formatting fixed; improved error message
fcb0146

fixed regression on enum test case
51b3f23

fixed expected error message in SignTests.fs
91b5c7e

fixed changed expected message in E_NoMethodsOnEnums01
3bd121f

Removed the last line of E_NoMethodsOnEnums01
0532bb0
since the second error it is introducing is not related to methods on enums and is difficult to verify.

another detail needed to change in an expected result in a test
b55ec10

Merge branch 'main' into issue83fix
56419ab

adding another test
ae427e3

Martin521 marked this pull request as ready for review December 11, 2023 21:04

vzarytovskii reviewed Dec 12, 2023
View reviewed changes

Comment threadsrc/Compiler/Checking/PostInferenceChecks.fs
Comment threadsrc/Compiler/TypedTree/TypedTree.fs
Comment threadsrc/Compiler/TypedTree/TypedTreePickle.fs
Comment threadsrc/Compiler/Checking/ConstraintSolver.fs
Comment threadsrc/Compiler/Driver/CompilerImports.fs

vzarytovskii approved these changes Dec 18, 2023
View reviewed changes

Merge branch 'main' into issue83fix
5fd3100

vzarytovskii closed this Dec 18, 2023

vzarytovskii reopened this Dec 18, 2023

Merge branch 'main' into issue83fix
54265bc

vzarytovskii merged commit 73970cb into dotnet:mainDec 20, 2023

auduchinok mentioned this pull request Jul 28, 2026
Import: Don't walk non-F# assemblies when labelling trait constraint sources #20090
Merged

Uh oh!

Conversation

Martin521 commented Nov 19, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Martin521 commented Nov 19, 2023

Uh oh!

github-actionsBot commented Nov 19, 2023

Uh oh!

psfinaki commented Nov 20, 2023

Uh oh!

Martin521 commented Nov 20, 2023

Uh oh!

smoothdeveloper commented Nov 20, 2023

Uh oh!

vzarytovskii commented Nov 20, 2023

Uh oh!

psfinaki commented Nov 20, 2023

Uh oh!

Martin521 commented Nov 21, 2023

Uh oh!

Martin521 commented Dec 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Martin521 commented Dec 18, 2023

Uh oh!

azure-pipelinesBot commented Dec 18, 2023

Uh oh!

T-Gro commented Dec 19, 2023

Uh oh!

azure-pipelinesBot commented Dec 19, 2023

Uh oh!

vzarytovskii commented Dec 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Martin521 commented Nov 19, 2023•
edited
Loading