[IDE] Report ambiguous cursor info results #63613

ahoppen · 2023-02-13T14:52:22Z

While we switched cursor info to solver-based I actually missed to return results in ambiguous cases. This PR fixes that oversight.

ahoppen · 2023-02-15T22:10:29Z

@swift-ci Please smoke test

ahoppen · 2023-02-15T22:10:32Z

@swift-ci Please SourceKit stress test

ahoppen · 2023-02-16T10:30:48Z

All stress tester failures are one of the following:

We fail to do local rename because the solver-based CursorInfo is able to resolve the cursor where the AST-based cursor info that we need for refactorings can’t. This is an improvement because previously neither rename nor cursor info worked, now only cursor info is working
There is a crash in SymbolGraph when doing Cursor Info on index in the stdlib. I think we can address this in a follow-up PR so we can merge this PR. A reduced test case is

func foo(hex: String) {
  // RUN: %sourcekitd-test -req=cursor -req-opts=retrieve_symbol_graph=1 -pos=%(line + 1):7 %s -- %s
  hex.index
}

rintaro · 2023-02-16T03:10:46Z

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

+        addCursorInfoForDecl(Data, Result, AddRefactorings, AddSymbolGraph,
+                             Lang, Invoc, Diagnostic, /*PreviousSnaps=*/{});
+      }
+      Data.DidReuseAST = Results->DidReuseAST;


This should be out of the loop.

rintaro · 2023-02-16T03:25:56Z

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

-                            Invoc, Diagnostic, /*PreviousSnaps=*/{},
-                            Results->DidReuseAST, Receiver);
+    CursorInfoData Data;
+    for (auto ResolvedCursorInfo : Results.getResult().ResolvedCursorInfos) {


Could we be consistent whether to use .getResults() or operator->?
(i.e. .getResult().ResolvedCursorInfos vs ->DidReuseAST)

Oh, I completely forgot that I added operator->

rintaro · 2023-02-16T15:51:05Z

lib/IDE/CursorInfo.cpp

+  /// and thus the constraint system solution doesn't know about the
+  /// \c UnresolvedDeclRefExpr. Instead, we find the expression to resolve in
+  /// the source file again after expression pre-check has run.
+  SourceFile &SrcFile;


Can this be the decl context instead? That should be faster.

ahoppen · 2023-02-17T09:59:39Z

@swift-ci Please smoke test

xedin · 2023-02-17T19:39:28Z

lib/Sema/TypeCheckConstraints.cpp

@@ -416,7 +416,11 @@ TypeChecker::typeCheckTarget(SolutionApplicationTarget &target,
  // Construct a constraint system from this expression.
  ConstraintSystemOptions csOptions = ConstraintSystemFlags::AllowFixes;

-  if (DiagnosticSuppression::isEnabled(Context.Diags))
+  /// If we have a SolutionCallback, we are inspecting constraint system
+  /// solutions directly and thus also want to receive ambiguous solutions.


This is actually interesting because there could be a number of valid solutions produced by the solver and I'm guessing we don't really want to run salvage in that case, right? It feels like this check needs to be embedded into the solver itself somehow...

You mean that we should just allow fixes straight away like we do for code completion?

I deliberately wanted to keep this as close to the normal type checking as possible (without all the code-completion specific rules of weaker filtering etc.) because if normal type checking reaches a single, definite result, we also want to report that as the single result for cursor info (for now). Also, in contrast to code completion, there is a reasonable chance that the code is in a valid state and thus it makes sense to run normal type-checking first and only salvage if we didn’t find a solution or the solutions were ambiguous.

I added this check to ConstraintSystem::solve now. Could you take another look at it?

ahoppen · 2023-03-14T00:00:23Z

@swift-ci Please smoke test

ahoppen · 2023-03-14T00:00:27Z

@swift-ci Please SourceKit stress test

ahoppen · 2023-03-17T17:49:51Z

@swift-ci Please smoke test

rintaro · 2023-03-17T18:31:01Z

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

+  Data.AvailableActions.insert(Data.AvailableActions.end(),
+                               KnownRefactoringInfo.begin(),
+                               KnownRefactoringInfo.end());


This could be append(KnownRefactoringInfo.begin(), KnownRefactoringInfo.end()), but I see the existing code used insert.

This hooks up the cursor info infrastructure to be able to pass through multiple, ambiguous results. There are still minor issues that cause solver-based cursor info to not actually report the ambiguous results but those will be fixed in a follow-up PR.

ahoppen · 2023-03-20T16:19:46Z

@swift-ci Please smoke test

ahoppen force-pushed the ahoppen/ambiguous-cursor-info-results branch 6 times, most recently from a5101fa to 471a0e2 Compare February 15, 2023 22:06

ahoppen changed the title ~~[IDE] Report ambiguous cursor info results 🚥 #63612~~ [IDE] Report ambiguous cursor info results Feb 15, 2023

ahoppen force-pushed the ahoppen/ambiguous-cursor-info-results branch from 471a0e2 to a9f0458 Compare February 15, 2023 22:10

ahoppen marked this pull request as ready for review February 15, 2023 22:11

ahoppen requested review from hborla, slavapestov and xedin as code owners February 15, 2023 22:11

rintaro reviewed Feb 16, 2023

View reviewed changes

ahoppen force-pushed the ahoppen/ambiguous-cursor-info-results branch from a9f0458 to ad932bd Compare February 17, 2023 09:59

xedin reviewed Feb 17, 2023

View reviewed changes

ahoppen force-pushed the ahoppen/ambiguous-cursor-info-results branch from ad932bd to 13f47b6 Compare March 13, 2023 23:59

ahoppen force-pushed the ahoppen/ambiguous-cursor-info-results branch from 13f47b6 to 36aa733 Compare March 17, 2023 03:32

ahoppen mentioned this pull request Mar 17, 2023

Symbolgraph crashes with Assertion failed: (NumFragments), function ~DeclarationFragmentPrinter, file DeclarationFragmentPrinter.h, line 143. #62457

Closed

ahoppen force-pushed the ahoppen/ambiguous-cursor-info-results branch from 36aa733 to cf26226 Compare March 17, 2023 17:49

rintaro approved these changes Mar 17, 2023

View reviewed changes

ahoppen added 3 commits March 20, 2023 08:09

[IDE] Report ambiguous cursor info results

eb67297

[CursorInfo] Fix issues found by the stress tester

e5c521c

ahoppen force-pushed the ahoppen/ambiguous-cursor-info-results branch from cf26226 to e5c521c Compare March 20, 2023 15:10

ahoppen merged commit 2108d01 into swiftlang:main Mar 20, 2023

ahoppen deleted the ahoppen/ambiguous-cursor-info-results branch March 20, 2023 20:07

[IDE] Report ambiguous cursor info results #63613

[IDE] Report ambiguous cursor info results #63613

Uh oh!

Conversation

ahoppen commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Feb 15, 2023

Uh oh!

ahoppen commented Feb 15, 2023

Uh oh!

ahoppen commented Feb 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Feb 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Mar 14, 2023

Uh oh!

ahoppen commented Mar 14, 2023

Uh oh!

ahoppen commented Mar 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Mar 20, 2023

Uh oh!

Uh oh!

ahoppen commented Feb 13, 2023 •

edited

Loading