Pass only narrow terminal-identifier range from name resolution#19818
Pass only narrow terminal-identifier range from name resolution#19818T-Gro wants to merge 8 commits into
Conversation
|
45a6bcf to
4f5da6f
Compare
|
(this adds on a recent PR, release notes were part of it already) |
| let resolvesAsExpr = | ||
| match nameResolutionResult with | ||
| | Result (tinstEnclosing, item, mItem, mItemIdent, rest, afterRes) | ||
| | Result (tinstEnclosing, item, mItem, rest, afterRes) |
There was a problem hiding this comment.
Can this be reverted to pre-#19505, so we don't reconstruct res argument below?
| let ad = env.eAccessRights | ||
| let typeNameResInfo = GetLongIdentTypeNameInfo delayed | ||
| let (tinstEnclosing, item, mItem, mItemIdent, rest, afterResolution) = | ||
| let (tinstEnclosing, item, mItem, rest, afterResolution) = |
There was a problem hiding this comment.
Can we remove this error post-processing?
| match ResolveExprLongIdent sink ncenv wholem ad nenv typeNameResInfo lid maybeAppliedArgExpr with | ||
| | Exception e -> Exception e | ||
| | Result (tinstEnclosing, item1, rest) -> | ||
| let itemRange, itemIdentRange = ComputeItemRange wholem lid rest |
There was a problem hiding this comment.
Can ComputeItemRange return the single range, as in pre-#19505?
There was a problem hiding this comment.
They are both needed - the other goes into sink(s) for IDE functions.
This is unexpected change and adds inconsistency with the breakpoints that tooling uses. We should try to keep the debugger info as is here. I think sequence points should use the whole expression ranges? Or do they rely on typed tree ranges that are now narrowed down? |
|
@auduchinok : It comes from the range on When stepping, why do you think it's wrong to highlight only the narrow item? Or is there another concern? |
The debugger sequence points normally cover the whole "statements" that include complete calls, arguments, and so on. Narrowing it here would create inconsistency in debugger experience. It would also be inconsistent with breakpoint ranges we provide in the tooling.
A guess: could it be because |
|
The narrowing only strips module/namespace/type prefixes from the sequence point:
But System.Console.WriteLine("hello") still gets a sequence point covering the entire call including "hello", because mWholeExpr unions the method range with the argument ranges. So I think OK? |
|
I really think we should not change sequence point ranges. I think we have to stay consistent with the rest of the language and C# and other languages debugging experience. Unfortunately, it seems we have to pass the two ranges to places like |
|
I also think we should keep the typed tree ranges closer to the syntax tree, so if some IDE feature work with this tree, they could also rely on these ranges. And we've been working hard on making syntax tree ranges correct 🙂 |
Simplifies the type-checking pipeline by removing the wide (module-qualified)
range from resolved-item returns. Only the narrow terminal-identifier range
flows through TcItemThen and downstream functions.
Affected features: error/warning squiggles now point at the member name
(e.g. 'Member') rather than the full qualified path ('Module.Member').
IDE hover, go-to-definition, and find-all-references use the narrow range
from NameResolution sinks (unchanged). No impact on IL emission or
binary compatibility — all changed APIs are internal.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Revert to pre-#19505 patterns: use '_ as res' in TcNameOfExpr and pass-through binding in TcLongIdentThen instead of destructuring and reconstructing the 5-tuple. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ow mItemIdent for diagnostics Restore itemRange (wide) alongside itemIdentRange (narrow) in name resolution return tuples. Thread mItemIdent through all TcItemThen handlers for attribute checks, error messages, and diagnostic squigglies while keeping mItem for expression construction and PDB sequence points. Revert test baselines for type unification and expression-range diagnostics to their original wide ranges. Update baselines for property/field access errors and other handler-specific diagnostics to their new narrow ranges. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- CheckRecdFieldMutation in static field path: mItem -> mItemIdent (parity with instance path) - nonStandardEventError in TcEventItemThen: mItem -> mItemIdent (parity with sibling event diagnostics) - Restore section comments in TcLookupItemThen for scanability Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rename mDiag -> mItemIdent in ApplyUnionCaseOrExn for naming consistency - Add clarifying comment at m m call site in TcUnionCaseOrExnField - Restore explanatory comments for duplicate diagnostics in OverloadResolutionErrorRangeTests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks! |
The fsharp suite migrated tests (neg10, neg15, neg17, neg90, neg91) use .bsl baselines that were not updated for the narrowed mItemIdent ranges. - neg10: Property 'not readable' diagnostics now point at property name - neg15: Union case/field access errors point at terminal identifier - neg17: Union case type accessibility error points at case name - neg90: RequireQualifiedAccess deprecation points at case name - neg91: Value accessibility/obsolete errors on assignment point at value Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Narrow diagnostic ranges to terminal identifiers
Follow-up to #19505 per review feedback.
Diagnostic ranges narrowed to terminal identifiers. Error/warning squiggles now point at the member name (
Red) rather than the full qualified path (Color.Red). The narrow range is computed once inComputeItemRangeand threaded asmItemIdentthrough allTc*ItemThenhandlers.TypedTree expression ranges preserved. Expression construction, type unification, PDB sequence points and debug stepping continue to use the wide
mItemrange — unaffected.Only code with qualified identifiers (
Module.Member,obj.Property,Namespace.Type.Member) sees narrower squiggles. Single-identifier code is unchanged.Unchanged: IDE hover · go-to-definition · find-all-references · IL emission · PDB sequence points · binary compatibility. No new/removed diagnostics, no severity changes.
Baseline changes — 24 ranges across 8 test files
All strictly narrower (start column shifts right, end column unchanged):
rec.fieldr1.foo→fooobj.Propertythis.GetInit→GetInitType.MemberColor.Red→RedClass.FieldClass.ObsoleteField→ObsoleteFieldClass.EventClass.ObsoleteEvent→ObsoleteEvent_.id(dot-lambda)_.id→idM.Svc.OldMethodOldMethod