-
Notifications
You must be signed in to change notification settings - Fork 788
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
Name resolution: keep type vars in subsequent checks #16456
Name resolution: keep type vars in subsequent checks #16456
Conversation
8003333
to
0b9c0ad
Compare
5a1e2d4
to
0b9c0ad
Compare
eb74aed
to
58a6e03
Compare
This is ready. Type arguments are preserved properly now in the captured items for types. 🎉 However, they aren't accessible via FCS APIs in some cases, so one of the added tests is disabled for now. I'm going to try to rework the type arguments story for I had to move a bit of code from ConstraintSolver to NameResolution along the way, but there were no changes to this code itself. |
190b071
to
e1679c4
Compare
❗ Release notes required
|
let synArgTysLength = synArgTys.Length | ||
let pathTypeArgsLength = pathTypeArgs.Length | ||
if tinst.Length <> pathTypeArgsLength + synArgTysLength then | ||
error (TyconBadArgs(env.DisplayEnv, tcref, pathTypeArgsLength + synArgTysLength, m)) | ||
|
||
let tps = tinst |> List.skip pathTypeArgsLength |> List.map (fun t -> | ||
match t with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't probably match on ttype directly, need do stripping first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure of this holds for a type instantiation, but it is a good point to add a test case with a type alias as a typar to see what it does.
If we stripped, we would lose the type alias and then it couldn't be searched for as a separate symbol... ?
There was a problem hiding this comment.
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'd be what we want to achieve here. I'm unwrapping the type variables produced by name resolution logic here, I probably don't want to change them in any way. Another approach could be to refactor more places and to pass the typar instantiation explicitly, but it seems that simply unwrapping it like this works for the cases we're interested in here, i.e. to make the types flow into subsequent checking phases after reporting.
Could this get merged, please? 🙂 |
* Name resolution: keep type vars in subsequent checks (#16456) * Keep typars produced in name resolution * Better debug errors * Unwrap measure type vars * Undo check declarations change * Fix reported range * Undo occurrence change * Skip path typars * Add test * More freshen typar APIs properly * Fantomas * Cleanup * Add release notes * 123 --------- Co-authored-by: Vlad Zarytovskii <[email protected]> * Build benchmarks in CI (#16518) * Remove profiling startpoint project * Add bench build job * Up * up * up --------- Co-authored-by: Kevin Ransom (msft) <[email protected]> * More ValueOption in compiler: part 1 (#16323) * More ValueOption in compiler: part 1 * release notes * Update CheckComputationExpressions.fs * release notes * `[Experimental]` `[WIP]` Transparent Compiler (#15179) * Track CheckDeclarations.CheckModuleSignature activity. (#16534) * Add Computation Expression Benchmarks (#16541) * add benchmarks for various usages of CEs * refactor * move CE source files to dedicated ce folder * Update Roslyn to a version which uses Immutable v7 (#16545) * revert #16326 (addition of XliffTasks reference) (#16548) * updated devcontainer image (#16551) * Add higher-order-function-based API for working with untyped AST (#16462) * Add module-based API for working with untyped AST * Fantomas * tryPickUntil → tryPickDownTo * Don't need that * Thread path while walking * Update comment * Simplify * Expose `Ast.fold` and `Ast.tryPick`. * Expose `SyntaxNode.(|Attributes|)`. * Ensure a few more syntax node cases get hit. * Update FCS release notes * Update surface area * Add back `foldWhile`; add `exists`, `tryNode` * Put `Ast.foldWhile` back in. * Add `Ast.exists`. * Add `Ast.tryNode`. * `SyntaxTraversal.Traverse` → `Ast.tryPick`… * Replace uses of `SyntaxTraversal.Traverse` in `FSharpParseFileResults` with the appropriate function from the `Ast` module: `exists`, `tryPick`, `tryNode`. * Update surface area * Need that * Just to be safe * Add `Ast.tryPickLast` * Handle multiple args mid-pipeline * Before, no signature help was offered in a case like this: ```fsharp [1..10] |> List.fold (fun acc _ -> acc) ‸ |> List.filter (fun x -> x > 3) ``` The service will now offer help for the `state` parameter when the cursor ‸ is in that location. * `*` instead of error * `FSharpParseFileResults.TryRangeOfFunctionOrMethodBeingApplied` was previously returning the range of the (zero-width) `SynExpr.ArbitraryAfterError`. It now returns the range of the `*` (`op_Multiply`) instead. * Update surface area * Fmt * Missed in merge * Add VS release notes entry * # → ### * Add ryPick tests * Add a few more tests * \n * Bump release notes * Fmt * `Ast` → `ParsedInput` * Use `ParsedInput` as the main AST type. * Move the `position` parameter rightward. * Update surface area * Less `function` * Update untyped AST docs * Add basic examples for `ParsedInput` module functions. * Merge the existing `SyntaxVisitorBase` docs into the new file. * Clean up doc comments --------- Co-authored-by: Vlad Zarytovskii <[email protected]> * Move paren entries to appropriate releases (#16561) * [main] Update dependencies from dotnet/source-build-reference-packages (#16532) * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240115.2 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24065.2 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240116.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24066.1 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240117.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24067.1 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240117.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24067.1 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240117.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24067.1 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240117.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24067.1 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Vlad Zarytovskii <[email protected]> * Attempt to make links from single identifier module names. (#16550) * Add scenarios where parentheses are around module name. * Address problem tighter to nameof usage. * Restore missing commit and inline nameof ident check. * Add release note entry. * rewrite SizeOfValueInfo in Optimizer.fs to be tail-recursive (#16559) * rewrite SizeOfValueInfo in Optimizer.fs to be tail-recursive * use Brians rewrite into one local function * stringbuilder is not threadsafe (#16557) * Array postfix notation in fsharp core api (#16564) * changed array types to postfix form in all signatures * changed array types to postfix form in the implementation files * Revert 16348 (#16536) * Improve AsyncMemoize tests * relax test condition * Revert "Cancellable: set token from node/async in features code (#16348)" This reverts commit d4e3b26. * remove UsingToken * remove UsingToken * test improvement * relax test condition * use thread-safe collections when collecting events from AsyncMemoize * fix flaky test * release note * Small code reshuffle for diff minimization (#16569) * Moving code around * Small code reshuffle for diff minimization * wat * Refactor parens API (#16461) * Refactor parens API * Remove `UnnecessaryParentheses.getUnnecessaryParentheses`. * Expose `SynExpr.shouldBeParenthesizedInContext`. * Expose `SynPat.shouldBeParenthesizedInContext`. * Expose `SyntaxTraversal.TraverseAll`. * Fantomas * Use `ParsedInput.fold` * Tests * Update surface area * Clean up sigs & comments * Update release notes * Remove redundant async * Remove stubs (no longer needed) * Preserve original stacktrace in state machines if available (#16568) * Preserve original stacktrace in state machines if available * Update release notes * Automated command ran: fantomas Co-authored-by: vzarytovskii <[email protected]> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * check reportErrors and feature support at top level (#16549) * Align DU case augmentation with previous behavior in EraseUnions (#16571) * Align DU case augment with previous behavior in EraseUnions * Update 8.0.300.md * modify tests * Refresh debug surface area (#16573) * Remove superfluous rec keywords and untangle some functions (#16544) * remove some superfluous rec keywords and untangle two functions that aren't mutually recursive. * Don't throw on invalid input in Graph construction (#16575) * More ValueOption in compiler: part 2 (#16567) * More ValueOption in complier: part 2 * Update release notes * extra optimization * extra optimization 2 * fantomas * Update dependencies from https://github.com/dotnet/arcade build 20240123.2 (#16579) Microsoft.DotNet.Arcade.Sdk From Version 8.0.0-beta.24060.4 -> To Version 8.0.0-beta.24073.2 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * [main] Update dependencies from dotnet/source-build-reference-packages (#16574) * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240122.5 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24067.1 -> To Version 9.0.0-alpha.1.24072.5 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240123.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24067.1 -> To Version 9.0.0-alpha.1.24073.1 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Tomas Grosup <[email protected]> * Improve AsyncMemoize tests (#16580) --------- Co-authored-by: Eugene Auduchinok <[email protected]> Co-authored-by: Vlad Zarytovskii <[email protected]> Co-authored-by: Petr <[email protected]> Co-authored-by: Kevin Ransom (msft) <[email protected]> Co-authored-by: Petr Pokorny <[email protected]> Co-authored-by: Florian Verdonck <[email protected]> Co-authored-by: dawe <[email protected]> Co-authored-by: Tomas Grosup <[email protected]> Co-authored-by: Martin <[email protected]> Co-authored-by: Brian Rourke Boll <[email protected]> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Jakub Majocha <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This is another approach for reporting type arguments, previously experimented in #14773 and #15628. In the following cases I'd like to make it possible to see the checked/inferred types of
I
type arguments:The previous approaches tried to overwrite reported symbols after the type arguments were checked, but it was quite ad-hoc and didn't work well with type parameters flow in cases like this one:
With that approach we'd need this:
This PR uses a different approach. It turns out that type variables were already always created for symbol reporting, but they were discarded afterwards and new type variables were created for subsequent type application checking. This is the reason why the initially reported type variables lacked inferred type solution: they never participated in the subsequent checks.
This PR makes the initially reported type variables flow through the subsequent type checking phases, so they have the correct solutions after the checking. This works for reporting types first and checking arguments later, because storing the type inference solutions uses mutation. With this approach, the type instantiations are correctly preserved in the cases like the one above.
This PR is not ready yet, and I want to check what tests fail in CI.