Attempt to make links from single identifier module names. #16550

nojaf · 2024-01-18T15:41:34Z

Description

I notice in some production code a missing link when using graph-based type-checking.

The problem is the following

In our line of thinking we always assumed that a single identifier needs to be preceded by an open statement before becoming available. That is why we don't check of single word identifiers in expressions and patterns if they can be matched.

In this screenshot, to consume module Foo in file B, you would either open Foo or prefix the usage of x (as Foo.x). So we either process the open statement or an identifier with two (or more) words.

Today, I found out that nameof ModuleName is a special case.
It can theoretically be linked to a node in the Trie.
Which is something we didn't foresee.

I can't think of any other examples besides nameof where this kind of behaviour is allowed, but to be safe I solved it without explicitly looking for the nameof presence.

@safesparrow could you please review this PR and let me know if this makes sense?

@vzarytovskii I know the window for 8.0.200 is probably closed, yet I'm asking you to consider merging this in the 200 branch. Otherwise, this fix will most likely only be available in May.

Checklist

Test cases added
Performance benchmarks added in case of performance changes
Release notes entry updated:
Please make sure to add an entry with short succinct description of the change as well as link to this pull request to the respective release notes file, if applicable.

Release notes files:
- If anything under src/Compiler has been changed, please make sure to make an entry in docs/release-notes/.FSharp.Compiler.Service/<version>.md, where <version> is usually "highest" one, e.g. 42.8.200
- If language feature was added (i.e. LanguageFeatures.fsi was changed), please add it to docs/releae-notes/.Language/preview.md
- If a change to FSharp.Core was made, please make sure to edit docs/release-notes/.FSharp.Core/<version>.md where version is "highest" one, e.g. 8.0.200.
Information about the release notes entries format can be found in the documentation.
Example:
- More inlines for Result module. (PR #16106)
- Correctly handle assembly imports with public key token of 0 length. (Issue #16359, PR #16363)
  *while! (Language suggestion #1038, PR #14238)
If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

github-actions · 2024-01-18T15:42:16Z

❗ Release notes required

✅ Found changes and release notes in following paths:

Change path Release notes path Description

src/Compiler docs/release-notes/.FSharp.Compiler.Service/8.0.300.md

vzarytovskii · 2024-01-18T15:56:02Z

@vzarytovskii I know the window for 8.0.200 is probably closed, yet I'm asking you to consider merging this in the 200 branch. Otherwise, this fix will most likely only be available in May.

I'm fine with that, but it won't get into 8.0.200 (it's closed already), but 20x. Would you please re-target it to 17.9, so it flows forward?

nojaf · 2024-01-18T16:51:50Z

After some feedback, this might be ok to fix in combination with nameof after all.
Unless anyone can come up with another example...

vzarytovskii · 2024-01-18T17:47:56Z

Ok, it has now bunch of commits from main, probably needs resetting from 17.9 and reapplying changes?

nojaf · 2024-01-19T07:50:57Z

@vzarytovskii I rebased the PR.

src/Compiler/Driver/GraphChecking/FileContentMapping.fs

nojaf · 2024-01-22T11:04:29Z

I've also talked to Janusz and he is ok with these changes.
He cannot respond to this PR as he has trouble logging into GitHub.

nojaf · 2024-01-22T12:10:55Z

Thanks all, I'd like to circle back to porting this to 8.0.201 once 8.0.200 is released.

vzarytovskii · 2024-01-22T17:29:44Z

/backport to release/dev17.9

github-actions · 2024-01-22T17:29:57Z

Started backporting to release/dev17.9: https://github.com/dotnet/fsharp/actions/runs/7615048087

github-actions · 2024-01-22T17:30:28Z

@vzarytovskii backporting to release/dev17.9 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Add scenarios where parentheses are around module name.
Applying: Address problem tighter to nameof usage.
Applying: Restore missing commit and inline nameof ident check.
Applying: Add release note entry.
Using index info to reconstruct a base tree...
A	docs/release-notes/.FSharp.Compiler.Service/8.0.300.md
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): docs/release-notes/.FSharp.Compiler.Service/8.0.300.md deleted in HEAD and modified in Add release note entry.. Version Add release note entry. of docs/release-notes/.FSharp.Compiler.Service/8.0.300.md left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0004 Add release note entry.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-01-22T17:30:29Z

@vzarytovskii an error occurred while backporting to release/dev17.9, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

* 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 <vzaritovsky@hotmail.com> * Build benchmarks in CI (#16518) * Remove profiling startpoint project * Add bench build job * Up * up * up --------- Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com> * 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 <vzaritovsky@hotmail.com> * 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 <vzaritovsky@hotmail.com> * 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 <1260985+vzarytovskii@users.noreply.github.com> --------- 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 <tomasgrosup@microsoft.com> * Improve AsyncMemoize tests (#16580) --------- Co-authored-by: Eugene Auduchinok <eugene.auduchinok@gmail.com> Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> Co-authored-by: Petr <psfinaki@users.noreply.github.com> Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com> Co-authored-by: Petr Pokorny <petrpokorny@microsoft.com> Co-authored-by: Florian Verdonck <florian.verdonck@outlook.com> Co-authored-by: dawe <dawedawe@posteo.de> Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com> Co-authored-by: Martin <29605222+Martin521@users.noreply.github.com> Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com> 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 <1760221+majocha@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Merge * Bump version * release notes * Versioning --------- Co-authored-by: Florian Verdonck <florian.verdonck@outlook.com>

) * 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.

nojaf requested a review from a team as a code owner January 18, 2024 15:41

nojaf changed the base branch from main to release/dev17.9 January 18, 2024 16:20

nojaf force-pushed the gbc/nameof-module branch from d8ffb9d to a7c1228 Compare January 19, 2024 07:50

auduchinok reviewed Jan 19, 2024

View reviewed changes

src/Compiler/Driver/GraphChecking/FileContentMapping.fs Outdated Show resolved Hide resolved

vzarytovskii enabled auto-merge (squash) January 19, 2024 13:17

vzarytovskii approved these changes Jan 19, 2024

View reviewed changes

nojaf added 3 commits January 22, 2024 11:41

Add scenarios where parentheses are around module name.

36cc692

Address problem tighter to nameof usage.

160f637

Restore missing commit and inline nameof ident check.

21e526d

auto-merge was automatically disabled January 22, 2024 10:41
Head branch was pushed to by a user without write access

nojaf force-pushed the gbc/nameof-module branch from 123b619 to 21e526d Compare January 22, 2024 10:41

nojaf changed the base branch from release/dev17.9 to main January 22, 2024 10:41

Add release note entry.

ce96db1

psfinaki approved these changes Jan 22, 2024

View reviewed changes

psfinaki merged commit 1112c11 into dotnet:main Jan 22, 2024
28 checks passed

nojaf mentioned this pull request Feb 21, 2024

Prefixed module nameof #16743

Merged

3 tasks

vzarytovskii added a commit that referenced this pull request Feb 22, 2024

Backport of #16550 to 8.0.202 (#16570)

4c93c63

* Merge * Bump version * release notes * Versioning --------- Co-authored-by: Florian Verdonck <florian.verdonck@outlook.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to make links from single identifier module names. #16550

Attempt to make links from single identifier module names. #16550

nojaf commented Jan 18, 2024

github-actions bot commented Jan 18, 2024 •

edited

Loading

vzarytovskii commented Jan 18, 2024

nojaf commented Jan 18, 2024

vzarytovskii commented Jan 18, 2024

nojaf commented Jan 19, 2024

nojaf commented Jan 22, 2024

nojaf commented Jan 22, 2024

vzarytovskii commented Jan 22, 2024

github-actions bot commented Jan 22, 2024

github-actions bot commented Jan 22, 2024

github-actions bot commented Jan 22, 2024

Attempt to make links from single identifier module names. #16550

Attempt to make links from single identifier module names. #16550

Conversation

nojaf commented Jan 18, 2024

Description

Checklist

github-actions bot commented Jan 18, 2024 • edited Loading

❗ Release notes required

vzarytovskii commented Jan 18, 2024

nojaf commented Jan 18, 2024

vzarytovskii commented Jan 18, 2024

nojaf commented Jan 19, 2024

nojaf commented Jan 22, 2024

nojaf commented Jan 22, 2024

vzarytovskii commented Jan 22, 2024

github-actions bot commented Jan 22, 2024

github-actions bot commented Jan 22, 2024

github-actions bot commented Jan 22, 2024

github-actions bot commented Jan 18, 2024 •

edited

Loading