[SourceKit] Fail requests when an error occurs #24124

DavidGoldman · 2019-04-18T16:21:30Z

Previously, requests would fail silently by returning an empty struct
in the response.

With this change, responses will properly report fail with the internal
error.

This allows IDEs to:

Properly identify errors when they occur
Choose to display the error to the user or perhaps handle it

Previously, requests would fail silently by returning an empty struct in the response. With this change, responses will properly report fail with the internal error.

akyrtzi · 2019-04-18T16:50:01Z

I like the idea of providing messages on why that specific source location did not resolve to a symbol but I'm not so sure about failing the request because of it. Request failures are more about the client providing a malformed request or something went wrong with sourcekitd (e.g. it crashed), they should not be happening if both the client and sourcekitd are operating properly.

DavidGoldman · 2019-04-18T17:27:43Z

What would the alternative be though? For all of these failures, the general information struct will be completely empty. Maybe a new response type should be added? It seems odd to add an error field to each potential response (which would only be set by itself).

FWIW, findRelatedIdents already had this behavior. I understand where you're coming from, but from an IDE's point of view the request did fail, it's just unclear if it's intentional.

akyrtzi · 2019-04-18T18:00:55Z

What would the alternative be though? For all of these failures, the general information struct will be completely empty. Maybe a new response type should be added? It seems odd to add an error field to each potential response (which would only be set by itself).

The cursor info response could get an extra field with a message that is set only when a symbol could not be resolved in that particular source location, as justification.

FWIW, findRelatedIdents already had this behavior.

Do you mean that findRelatedIdents is failing the request if you don't point to a symbol ? I'm not able to reproduce this.

I'm not aware of a request that works like this, for example code-completion is not failing the request if it doesn't find any symbols to return at a particular source location.

I understand where you're coming from, but from an IDE's point of view the request did fail, it's just unclear if it's intentional.

The IDE can already see that no symbol is present at that specific source location, if it wants to also show a message or log it for debugging purposes it can pick it up from the the extra field.

DavidGoldman · 2019-04-18T18:29:01Z

What would the alternative be though? For all of these failures, the general information struct will be completely empty. Maybe a new response type should be added? It seems odd to add an error field to each potential response (which would only be set by itself).

The cursor info response could get an extra field with a message that is set only when a symbol could not be resolved in that particular source location, as justification.

FWIW, findRelatedIdents already had this behavior.

Do you mean that findRelatedIdents is failing the request if you don't point to a symbol ? I'm not able to reproduce this.

Ah my bad, it's actually editorFindInterfaceDoc that has this behavior (although the Error is included in the info). See here.

I'm not aware of a request that works like this, for example code-completion is not failing the request if it doesn't find any symbols to return at a particular source location.

I understand where you're coming from, but from an IDE's point of view the request did fail, it's just unclear if it's intentional.

The IDE can already see that no symbol is present at that specific source location, if it wants to also show a message or log it for debugging purposes it can pick it up from the the extra field.

Gotcha. I think part of this change makes sense to keep as failure: e.g. bad invocations, if you think that the cursor info in particular should be changed to add a field as well let me know.

akyrtzi · 2019-04-18T18:44:32Z

I think bad invocations (e.g. missing compiler arguments) are fine to fail the request. To make the distinction (should something fail the request or not) think of it like this:

If it is possible to know in advance that the request is going to fail (e.g. some field is missing, or the compiler arguments don't include the file you want information from), then this is failable. If there is no way for the client to know in advance just by looking at the request (e.g. there is no symbol at a source location, or no completion results at that location), then the response should not be that the request itself is erroneous.

DavidGoldman · 2019-04-18T19:16:28Z

I think bad invocations (e.g. missing compiler arguments) are fine to fail the request. To make the distinction (should something fail the request or not) think of it like this:

If it is possible to know in advance that the request is going to fail (e.g. some field is missing, or the compiler arguments don't include the file you want information from), then this is failable. If there is no way for the client to know in advance just by looking at the request (e.g. there is no symbol at a source location, or no completion results at that location), then the response should not be that the request itself is erroneous.

What do you think of the case when an internal error occurs (something unexpected happened)? Still don't fail the request?

Anyways, then for both the CursorInfo and NameInfo, a new Error field should be added? Or should I just leave it as is with an empty response?

akyrtzi · 2019-04-18T20:15:17Z

What do you think of the case when an internal error occurs (something unexpected happened)? Still don't fail the request?

They should fail the request, abnormal situations like sourcekitd crashing, or other unexpected things, should fail the request.

Anyways, then for both the CursorInfo and NameInfo, a new Error field should be added? Or should I just leave it as is with an empty response?

How about adding an internal_diagnostic string field in the response that will be present in situations where the response is currently empty ?

DavidGoldman · 2019-04-19T15:20:39Z

What do you think of the case when an internal error occurs (something unexpected happened)? Still don't fail the request?

They should fail the request, abnormal situations like sourcekitd crashing, or other unexpected things, should fail the request.

Anyways, then for both the CursorInfo and NameInfo, a new Error field should be added? Or should I just leave it as is with an empty response?

How about adding an internal_diagnostic string field in the response that will be present in situations where the response is currently empty ?

Yeah that sounds reasonable, I'll go ahead and add that

DavidGoldman · 2019-04-19T18:09:40Z

Added the InternalDiagnostic, please take another look

DavidGoldman · 2019-04-29T16:35:37Z

Friendly ping

akyrtzi · 2019-05-01T22:56:35Z

test/SourceKit/CursorInfo/invalid_offset.swift

@@ -12,4 +12,4 @@ let a = 12
 // rdar://problem/38162017
 // REQUIRES: OS=macosx

-// CHECK: <empty cursor info>
+// CHECK: (Request Failed): Unable to resolve the start of the token


@rintaro, in your fix for rdar://30346106 you had the request return an empty response for out-of-range offset, are you ok with the response failing instead ?

I believe it should be OK.

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

tools/SourceKit/tools/sourcekitd/lib/API/Requests.cpp

Previously, requests would fail silently by returning an empty struct in the response. With this change, responses will properly report fail with the internal error.

DavidGoldman · 2019-05-03T19:48:48Z

Made the changes you requested, please let me know if the RequestResult looks good as I'm not super familiar with C++

DavidGoldman · 2019-05-15T13:54:03Z

Friendly ping

gottesmm · 2019-05-15T18:53:50Z

(Just an FYI, @DavidGoldman I did a friendly re-request review drive by.)

tools/SourceKit/include/SourceKit/Core/LangSupport.h

benlangmuir · 2019-05-20T18:15:43Z

tools/SourceKit/tools/sourcekitd-test/sourcekitd-test.cpp

+      sourcekitd_variant_dictionary_get_string(Info, KeyInternalDiagnostic);
+  if (InternalDiagnostic) {
+    OS << "<diagnostic \"" << InternalDiagnostic << "\">\n";
+    return;


To me, this makes it look like a (true) failure instead of just an empty result. I suggest something like

<empty cursor info> <internal diagnostic "...">

Or

<empty cursor info; internal diagnostic: "...">

tools/SourceKit/include/SourceKit/Core/LangSupport.h

DavidGoldman · 2019-05-21T18:55:11Z

Made the requested fixes

akyrtzi

Thank you for working on this!

akyrtzi · 2019-05-21T19:02:17Z

@swift-ci smoke test

DavidGoldman · 2019-05-22T17:30:33Z

No problem! let me know if i need to merge/rebase onto master

akyrtzi · 2019-05-22T17:51:18Z

Could you open a PR for swift-5.1 branch ?

DavidGoldman · 2019-05-22T18:02:31Z

Will do, how much time do you think I have though?

akyrtzi · 2019-05-22T18:08:24Z

There's time; worst can happen is we reject it if it is too late.

jckarter · 2019-05-22T22:18:24Z

This is causing a bunch of tests to fail in CI:

https://ci.swift.org/job/oss-swift-incremental-ASAN-RA-osx/3368/consoleText

Failing Tests (7):
Swift(macosx-x86_64) :: SourceKit/Refactoring/syntactic-rename.swift
Swift(macosx-x86_64) :: SourceKit/Misc/edit_range.swift
Swift(macosx-x86_64) :: SourceKit/Indexing/index_module_missing_depend.swift
Swift(macosx-x86_64) :: SourceKit/DocSupport/doc_source_file.swift
Swift(macosx-x86_64) :: SourceKit/CursorInfo/invalid_offset.swift
Swift(macosx-x86_64) :: SourceKit/CompileNotifications/arg-parsing.swift
Swift(macosx-x86_64) :: SourceKit/CodeComplete/complete_open.swift

I'm going to revert.

[SourceKit] Fail requests when an error occurs

70b36fe

Previously, requests would fail silently by returning an empty struct in the response. With this change, responses will properly report fail with the internal error.

Add InternalDiagnostic to CursorInfoData and NameTranslatingInfo

aad8609

akyrtzi reviewed May 1, 2019

View reviewed changes

akyrtzi requested changes May 1, 2019

View reviewed changes

DavidGoldman added 6 commits May 3, 2019 11:01

[SourceKit] Fail requests when an error occurs

10ecea6

Previously, requests would fail silently by returning an empty struct in the response. With this change, responses will properly report fail with the internal error.

Add InternalDiagnostic to CursorInfoData and NameTranslatingInfo

a3a4d2d

RequestResult class and Pass*InfoForDecl fixes

00d771c

Merge branch 'master' of https://github.com/apple/swift

970ffab

Merge

9a9854b

Fix bad merge

6d59776

gottesmm requested a review from akyrtzi May 15, 2019 18:53

benlangmuir reviewed May 20, 2019

View reviewed changes

akyrtzi reviewed May 20, 2019

View reviewed changes

tools/SourceKit/include/SourceKit/Core/LangSupport.h Outdated Show resolved Hide resolved

tools/SourceKit/include/SourceKit/Core/LangSupport.h Outdated Show resolved Hide resolved

tools/SourceKit/include/SourceKit/Core/LangSupport.h Show resolved Hide resolved

Comment and sourcekitd-test fixes

1c81982

akyrtzi approved these changes May 21, 2019

View reviewed changes

akyrtzi merged commit 729a955 into swiftlang:master May 22, 2019

jckarter mentioned this pull request May 22, 2019

Revert "[SourceKit] Fail requests when an error occurs" #24993

Merged

DavidGoldman mentioned this pull request May 28, 2019

[SourceKit] Fail requests when an error occurs #25089

Merged

[SourceKit] Fail requests when an error occurs #24124

[SourceKit] Fail requests when an error occurs #24124

Uh oh!

Conversation

DavidGoldman commented Apr 18, 2019

Uh oh!

akyrtzi commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavidGoldman commented Apr 18, 2019

Uh oh!

akyrtzi commented Apr 18, 2019

Uh oh!

DavidGoldman commented Apr 18, 2019

Uh oh!

akyrtzi commented Apr 18, 2019

Uh oh!

DavidGoldman commented Apr 18, 2019

Uh oh!

akyrtzi commented Apr 18, 2019

Uh oh!

DavidGoldman commented Apr 19, 2019

Uh oh!

DavidGoldman commented Apr 19, 2019

Uh oh!

DavidGoldman commented Apr 29, 2019

Uh oh!

akyrtzi May 1, 2019

Choose a reason for hiding this comment

Uh oh!

rintaro May 1, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DavidGoldman commented May 3, 2019

Uh oh!

DavidGoldman commented May 15, 2019

Uh oh!

gottesmm commented May 15, 2019

Uh oh!

Uh oh!

benlangmuir May 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DavidGoldman commented May 21, 2019

Uh oh!

akyrtzi left a comment

Choose a reason for hiding this comment

Uh oh!

akyrtzi commented May 21, 2019

Uh oh!

DavidGoldman commented May 22, 2019

Uh oh!

akyrtzi commented May 22, 2019

Uh oh!

DavidGoldman commented May 22, 2019

Uh oh!

akyrtzi commented May 22, 2019

Uh oh!

jckarter commented May 22, 2019

Uh oh!

Uh oh!

akyrtzi commented Apr 18, 2019 •

edited

Loading