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

improve compiler error message for failed overload resolution #6596

Merged
merged 96 commits into from
Feb 19, 2020

Conversation

smoothdeveloper
Copy link
Contributor

@smoothdeveloper smoothdeveloper commented Apr 21, 2019

Work related to method overload resolution error messages: #6578.

  • implementing changes to match the original description
    • find other error message cases
  • add sufficient test coverage in the new test suite
    • overloads with out / ref / byref parameters (covered by some fsharpqa migrated tests)
    • check the impact of generic in error display (covered by some fsharpqa migrated + new tests)
    • no redundant spacing among the sections of the message (checked in situ and enforced in .bsl files)
    • add more cases, involving same technique as FSharpPlus (or else) and making sure that relevant type mistmatch information is not lost
  • consider listing the overloads in case of argument count mismatch (right now there is some logic to propose a correct number of parameters) -> separate PR
  • review of PR -> MSFT
  • finalize wording / name of errors in the code
  • integrate review feedback
  • investigate the documentation / guides for parts that could be impacted by error message changes -> MSFT

@smoothdeveloper smoothdeveloper force-pushed the overload-list-error-message branch 2 times, most recently from f577797 to e2d0c20 Compare April 21, 2019 16:44
@smoothdeveloper
Copy link
Contributor Author

Few notes for myself & reviewers that would like to validate my approach.

I've been stepping around the overload resolution code, for now I'm considering this option for implementation:

  • Change ConstraintSolver.ResolveOverloading callerArgCounts argument into callerArgs at this call site

https://github.com/Microsoft/visualfsharp/blob/c4c1d9cf1097955a71c7cbc328a6b5c672a34bda/src/fsharp/TypeChecker.fs#L9889-L9900

(need to check the other call site to ResolveOverloading happening in SolveMemberConstraint how change this argument can be handled)

  • using this information, assemble the top of the message, which will contain the method application given parameter types in ResolveOverloading's failOverloading local function.

  • continue assembling the rest of the message where it is currently handled in CompileOps.OutputPhasedErrorR appending the overload list in a compact form

I was considering building the whole message in CompileOps.OutputPhasedErrorR but the change can be implemented without touching this aspect and make review easier.

@smoothdeveloper
Copy link
Contributor Author

smoothdeveloper commented Apr 22, 2019

I'm trying to understand the reason for callerArgs type to be (CallerArg<Expr> list * CallerNamedArg<Expr> list) list: a list of tuple of lists.

https://github.com/Microsoft/visualfsharp/blob/c4c1d9cf1097955a71c7cbc328a6b5c672a34bda/src/fsharp/TypeChecker.fs#L9879

I'd see the caller arguments as either:

  • (CallerArg | CallerNamedArg) list
  • (CallerArg list * CallerNamedArg list)

I'm not sure what is the extra nesting in a list is used for?

edit: consolidating into CallerArgs struct record so the operations and constant values can be encapsulated rather than being implementation details in many places.

@smoothdeveloper
Copy link
Contributor Author

Just a note listing the fsharpqa tests that started failing when I changed the error message a bit (https://dev.azure.com/dnceng/public/_build/results?buildId=164032).

I want to investigate how those flow through ConstraintSolver.ResolveOverloading.

Conformance\Expressions\SyntacticSugar (E_Slices01.fs.fs) -- failed
Conformance\InferenceProcedures\TypeInference (E_OneTypeVariable03.fs) -- failed
Conformance\InferenceProcedures\TypeInference (E_OneTypeVariable03rec.fs) -- failed
Conformance\InferenceProcedures\TypeInference (E_TwoDifferentTypeVariables01.fs) -- failed
Conformance\InferenceProcedures\TypeInference (E_TwoDifferentTypeVariables01rec.fs) -- failed
Conformance\InferenceProcedures\TypeInference (E_TwoDifferentTypeVariablesGen00rec.fs) -- failed
Conformance\InferenceProcedures\TypeInference (E_TwoEqualTypeVariables02.fs) -- failed
Conformance\InferenceProcedures\TypeInference (E_TwoEqualYypeVariables02rec.fs) -- failed
Conformance\InferenceProcedures\TypeInference (E_LeftToRightOverloadResolution01.fs) -- failed
Conformance\InferenceProcedures\WellFormednessChecking (E_Clashing_Values_in_AbstractClass01.fs) -- failed
Conformance\InferenceProcedures\WellFormednessChecking (E_Clashing_Values_in_AbstractClass03.fs) -- failed
Conformance\InferenceProcedures\WellFormednessChecking (E_Clashing_Values_in_AbstractClass04.fs) -- failed
ErrorMessages\OverloadResolution (E_System.Convert.ToString.OverloadList) -- failed

@smoothdeveloper
Copy link
Contributor Author

open/close principle for CI

@smoothdeveloper smoothdeveloper force-pushed the overload-list-error-message branch 5 times, most recently from bcc468c to b258027 Compare April 25, 2019 16:53
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this pull request Apr 30, 2019
…cted by dotnet#6596

the baseline files are empty on purpose, expecting CI failure reporting those tests, I intend to update the baseline and clean up the comments / xml tags from fsharpqa syntax in later commit, and then remove those specific tests altogether from fsharpqa if this is OK with the maintainers.
brettfo pushed a commit that referenced this pull request Apr 30, 2019
…n error messages) (#6654)

* migrate (as a separate copy) fsharpqa tests that are going to be impacted by #6596

the baseline files are empty on purpose, expecting CI failure reporting those tests, I intend to update the baseline and clean up the comments / xml tags from fsharpqa syntax in later commit, and then remove those specific tests altogether from fsharpqa if this is OK with the maintainers.

* update the .bsl files

* remove comments that are handled by baseline files, update baseline files

* remove the migrated tests from fsharpqa tests

* need to be more careful when migrating those

* testing if running the test with .fs instead of .fsx makes them work on .netcore

* exclude migrated fsharpqa from dotnet core run
@smoothdeveloper
Copy link
Contributor Author

the work is progressing, there are new tests failling in fsharpqa, that i'll probably move in this PR

Conformance\Expressions\Type-relatedExpressions (E_RigidTypeAnnotation03.fsx) -- failed
Conformance\InferenceProcedures\TypeInference (E_TwoDifferentTypeVariablesGen00.fs) -- failed
Conformance\LexicalAnalysis\SymbolicOperators (E_LessThanDotOpenParen001.fs) -- failed

image

This is going through a code path I haven't yet stepped through (units of measure show in the test) and need adjustment to the message building logic, I'll look further toward the end of the week.

@cartermp
Copy link
Contributor

cartermp commented May 2, 2019

@smoothdeveloper Wonderful!

@smoothdeveloper
Copy link
Contributor Author

VS error box (left=after,right=before):
vs error box

VS Tooltip before:
vs tooltip before
VS Tooltip after:
vs tooltip after

VS Tooltip (alternate message, filtered on ref types) before (single line message in tooltip woes):
vs tooltip reftype before

VS Tooltip (alternate message, filtered on ref types) after:
vs tooltip reftype after

The experience in FSI is also good.

I've fixed the windows desktop failing tests but now new fsharpqa test is failing. Pushing some minor changes to do a full run again.

@cartermp
Copy link
Contributor

cartermp commented May 2, 2019

@smoothdeveloper Absolutely wonderful!

@smoothdeveloper
Copy link
Contributor Author

This is green but I had to disable the fsharpqa optimization / inlining test: 8514f09

@smoothdeveloper smoothdeveloper changed the title [wip] improve compiler error message for failed overload resolution improve compiler error message for failed overload resolution May 8, 2019
@smoothdeveloper
Copy link
Contributor Author

Green again and ready for review.

…could not be determined based on type information prior to this program point. A type annotation may be needed." if there are no optional parts.
@smoothdeveloper
Copy link
Contributor Author

I've removed the xlf from the diff, they should be added to .gitignore, and in case external contributors want to contribute translations, a guideline written to enable it; as of now, xlf seems like an internal MSFT thing with no impact on contributions to this repository beside generating more questions / diff / merge conflicts.

@dsyme, I've fixed the recent conflict and it is (allegedly soon) green again.

@cartermp said on slack it were now dependent on your aproval and I feel our degree of confidence about the changes have passed the bar, also the change is as minimal as we'd want to not disrupt other branches were work is happening.

I'll definitely be around on simple ping for any adjustment needed on top of this work if needed, in separate PRs, but maintaing this long lived branch through merged changes I don't have good grasp on is becoming harder and increasing the overhead of impact versus cumulated time spent by everyone involved.

Once merged, Ionide and Rider users will also benefit from the changes once they land in FCS, leading to potentially more input / feedback from end users before this ships in Visual Studio / dotnet SDK.

The efforts I spend on rebasing without glitching / losing anything, and the lack of better messages in ongoing work dealing with method overload are adding extra incentives to get it merged, let's do it 🙂.

@smoothdeveloper
Copy link
Contributor Author

It is really green.

src/fsharp/MethodCalls.fs Outdated Show resolved Hide resolved
Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

I'm marking this as approved - it looks like there is one old comment to remove though?

@smoothdeveloper
Copy link
Contributor Author

Comment removed, thanks @dsyme!

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

I like the overall change and feel generally comfortable with the number of suggestions, since it's quite rare to have an enormous amount of overloads and that is something that could be adjusted over time.

Another thing to consider for a separate PR would be to integrate these suggestions with tooling. There we'd have to trim the set of suggestions, since 10+ items in a quick fix menu doesn't work. But that's a separate concern I think.

@cartermp
Copy link
Contributor

@KevinRansom wanna take a look?

@KevinRansom KevinRansom merged commit acd7cfd into dotnet:master Feb 19, 2020
@cartermp
Copy link
Contributor

Yeet

@KevinRansom
Copy link
Member

Is yeet good or bad ? I thought it was throwing things of a cliff?

@baronfel
Copy link
Member

sometimes you have to yeet things to see them fly

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
…#6596)

* migrate (as a separate copy) fsharpqa tests that are going to be impacted by dotnet#6596

the baseline files are empty on purpose, expecting CI failure reporting those tests, I intend to update the baseline and clean up the comments / xml tags from fsharpqa syntax in later commit, and then remove those specific tests altogether from fsharpqa if this is OK with the maintainers.

* update the .bsl files

* remove comments that are handled by baseline files, update baseline files

* remove the migrated tests from fsharpqa tests

* need to be more careful when migrating those

* testing if running the test with .fs instead of .fsx makes them work on .netcore

* exclude migrated fsharpqa from dotnet core run

* sample test in fsharpqa (can't run locally for now)

* trying to make it green now.

* checking if this path is covered by a test, trying to identify how to trigger this other overload related error message

* * [MethodCalls.fs] Defining CallerArgs<'T> in, this replaces passing of callerArgsCount, uncurriedCallerArgs and other variants in the overload resolution logic happening in ConstraintSolver & TypeChecker
* [TypeChecker.fs] pass CallerArgs instace at call sites for overload resolution + some commented code as we'll be building a list of given argument types (probably moving as a CallerArgs method or property)
* [ConstraintSolver.fs/fsi] pipe the overload resolution traced callback to `trace.CollectThenUndoOrCommit` as that expression is long and more important in that context

* bit of refactoring of error message building logic during failed overload resolution

[ConstraintSolver.fsi]

* define OverloadInformation and OverloadResolutionFailure, those replace workflow where `string * ((CalledMeth<_> * exn) list)` values were created at call site mixed with message building and matched later in non obvious fashion in `failOverloading` closure
* adjust UnresolvedOverloading and PossibleOverload exceptions

[ConstraintSolver.fs]

* get rid of `GetPossibleOverloads`, this was used only in `failOverloading` closure, and just being passed the same parameters at call site
* give `failOverloading` a more meaningful signature and consolidate the prefix message building logic there
* fix `failOverloading` call sites

[CompilerOps.fs] adjust pattern matching

Expecting behaviour of this code to be identical as before PR

`

* (buildfix) harmonizing .fsi/.fs right before commit doesn't always work with simple copy paste.

* trying to check what kind of things break loose when I change this

I'm trying to identify why we get `(CalledMeth<Expr> * exn list)`, I'm making a cartesian product through getMethodSlotsAndErrors for now, trying to figure out if we can get other than 0 or 1 exception in the list for a given method slot.

I may revert that one if it doesn't make sense make from a reviewer standpoint and/or breaks fsharpqa tests surounding overload resolution error messages.

* (minor) [ConstraintSolver.fs] revert space mishapps to reduce diff, put back comments where they were

* toward displaying the argument names properly (may fail some fsharpqa tests for now)

* missing Resharper's Ctrl+Alt+V "introduce variable" refactoring @auduchinok :)

* pretty print unresolved overloads without all the type submsumption per overload verbose messages

* Overload resolution error messages: things display like I want in fsi, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.

* adjust message for candidates overload

* Hijack the split phase for UnresolvedOverloading, remove the match on PossibleOverload.

It consolidates some more of the string building logic, and it now shows as a single compact and exhaustive error message in VS

Thinking I'll change PossibleOverload to not be an exception if going this way is OK

* updating existing failing baseline files that looks correct to me

* quickfix for update.base.line.with.actuals.fsx so that it skips missing base line files in the loop

* (minor) minimize diff, typos, comments

* fix vsintegration tests affected by overload error message changes

* merge issue unused variable warning

* update the 12 fsharpqa migrated baseline with new error messages

* (minor) baseline update script

[update.base.line.with.actuals.fsx]
* add few directories
* error handling (when tests are running, the .err files are locked

* move System.Convert.ToString and System.Threading.Tasks.Task.Run tests from QA

* * moving 3 fsharpqa tests to new test suite
* bring back neg_System.Convert.ToString.OverloadList.fsx which I mistakenly removed during merge

* consolidate all string building logic in CompileOps.fs, fix remaining cases with missing \n (the end result code is less whack-a-mole-ish)

* update base lines

* remove the migrated tests from fsharpqa

* fix vstest error message

* fix env.lst, removed wrong one...

* update baselines of remaining tests

* adding one simple test with many overloads

* appropriate /// comments on `CalledMeth` constructor arguments

* minimize diff / formatting

* trim unused code

* add simple test, one message is interesting, mentioning parameter count for possible overloads

* comment flaky test for now

* code review on the `coerceExpr` function from @TIHan, we can discard first `isOutArg` argument as the only hardcoded usage was guarded by `error` when the value is true, and passing an explicit false which is now guaranteed equivalent to `callerArg.IsOptional`

* code formatting remarks on ConstraintSolver.fs/fsi

* (minor) formatting

* format known argument type / updating .bsl

* update missing baseline

* update missing baselines

* update missing baseline

* minor: make TTrait better in debugger display

* [wip] pass TraitConstraintInfo around failed overload resolution error, and try to display some of it's info

* minimize diff

* signature file mismatch

* removing duplicate in fscomp.txt

* surfacing initial bits of TraitConstraintInfo, roughly for now

* formatting types of known argument in one go, the formatting is still wrong:

* unamed type arguments aren't relabelled (still show their numerical ids)
* SRTP constraints are dismissed, not sure how they should be lined up to pass to SimplifyTypes.CollectInfo

* rework of overload failure message to prettify *ALL* types in a single go, not only argument types, but return types and known generic argument types (this info was never displayed originally but is useful for scenario similar to FSharpPlus implementation)

added `PrettifyDiscriminantAndTypePairs` in TastOps, this allow to weave extra information with the complete list of types, to help reconstituting the several types from their origin (in the usage spot: argument types, return type, generic parameter types)

* fixup the tests and add two tests

* one checking on the new information of known return type and generic parameter types
* one checking we don't leak internal unammed Typar and that they don't all get named 'a despite being different

* updating baselines that got tighter.

* simplify handling of TraitConstraintInfo

* naming couple of fields and turning a property to a methods as it triggers an assert in debugger when inspecting variables

* comments in the assembling of overload resolution error message

* Add information about which argument doesn't match
Add couple of tests
Updating bunch of baselines

* minor updates to testguide and devguide

* fix PrimitiveConstraints.``Invalid object constructor`` test

* fix(?) salsa tests

* minimize diff

* put back tests under !FSHARP_SUITE_DRIVES_CORECLR_TESTS

* missing updated message

* minor adjustments to TESTGUIDE.md

* return type was missing prettifying in prettyLayoutsOfUnresolvedOverloading

* address code review nits / minimize diff / add comment on PrettifyDiscriminantAndTypePairs

* minimize diff

* proposed work around the flaky error message until dotnet#6725 has a fix

we keep the fsharpqa test around (but removing the overload error messages from what is asserted out of it) in the meantime

* fixing baselines of new tests from master

* sisyphus round of baseline update

* removing type which isn't in use and popped back up after rebase

* minimize diff

* tidy inconsistent tuple literal

* * removing TTrait properties that end up not being used
* renaming tys and returnTy fields to better match convention (`tys` is used, so no underscore prefix)
* minimizing diff

* minimize diff

* minimize diff

* minimize diff

* link to usage example in same file

* revert converting CallerArg single cased DU into Record

* minimize diff

* minimize diff

* minimize diff

* fix rebase glitches

* fix rebase glitch

* update baseline

* fix base lines / new tests base lines

* edge case: needs a new line after "A unique overload for method '%s' could not be determined based on type information prior to this program point. A type annotation may be needed." if there are no optional parts.

* updating base line for edge case of missing new line

* missing baseline

* removing comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants