Add a BuildSystemDelegate which supports notifications for build settings changes

[DavidGoldman]

Introduce a BuildSystemDelegate to handle notifications from the build system

• SourceKitServer is the main delegate to process these notifications
• Currently limited to changes in FileBuildSettings
• Delegate informs the BuildSystem of files to watch via registerChangeWatching(for: URL) and unregisterChangeWatching(for: URL)
• In the future we could have more integration for handling changes in dependencies

Handling changes in FileBuildSettings

• SourceKitServer sends notifications to the internal LSPs informing them of any opened documents that have changes in their compiler flags
  • For clangd, we send a notification to update the compilation database
  • For SourceKit/sourcekitd we must close and reopen the file to force a new AST with the new compiler flags

[DavidGoldman]

Heavily based off of #147, evolved from the code there and discussion here.

[benlangmuir]

Could you please squash this so we don't have a merge in the middle (or at least squash the changes up to and including the merge commit)?

[benlangmuir]

We don't need to nail all of this down right away, but I'm wondering about the split of responsibilities between notification and settings method.

• What is the expectation for the performance of calling settings(for:_:) after you get a notification? Can it ever still be slow? What about before getting a notification, such as on initial query?
• Would it be reasonable to provide the settings right in the notification instead so you don't need to make another call?
• Should that be the primary way to get settings, and you just always get an initial notification when you register? That ways the standalone settings(for:_:) method is only intended for one-off requests, not to be mixed with notifications.
• On the other hand, when you open a new document, it's nice to call settings(for:_:) and get initial settings before e.g. talking to sourcekitd, but maybe the real thing you want is to get the initial settings with a timeout so that if it's going to take a few seconds to load settings you just open the sourcekitd document without settings and update it when they're available.

[DavidGoldman]

Squashed, let me know if it looks OK.

We don't need to nail all of this down right away, but I'm wondering about the split of responsibilities between notification and settings method.

• What is the expectation for the performance of calling settings(for:_:) after you get a notification? Can it ever still be slow? What about before getting a notification, such as on initial query?

That's a good point. What do you think? One option is for the BuildSystem to return some placeholder flags and then fix them up later (by sending a notification). Alternatively the BuildSystem could block trying to resolve these, but who knows how long it could take.

I would assume that after a notification, the BuildSystem likely has the flags cached (unless they were invalidated - e.g. the BuildSystem no longer has a record of a file).

• Would it be reasonable to provide the settings right in the notification instead so you don't need to make another call?

Yeah, this seems reasonable. Potentially an optional, where null means the BuildSystem no longer has a record of the file?

• Should that be the primary way to get settings, and you just always get an initial notification when you register? That ways the standalone settings(for:_:) method is only intended for one-off requests, not to be mixed with notifications.
• On the other hand, when you open a new document, it's nice to call settings(for:_:) and get initial settings before e.g. talking to sourcekitd, but maybe the real thing you want is to get the initial settings with a timeout so that if it's going to take a few seconds to load settings you just open the sourcekitd document without settings and update it when they're available.

Yeah, it's unclear what to do here if the BuildSystem might take some time to load it. Maybe settings(for:_:) can be shifted to be async (with a callback) to allow for this type of timeout?

[benlangmuir]

One option is for the BuildSystem to return some placeholder flags and then fix them up later (by sending a notification).

I'm wary of using fallback settings in the interim, because it would cause us to create a semantic AST in sourcekitd that is almost certain to have errors. So you're wasting resources typechecking an AST, and you end up with bad diagnostics like failing to import every module. I think you only want to use fallback settings when the build system doesn't know about the file. You could instead open the document with no settings so that you get syntactic support right away.

Potentially an optional, where null means the BuildSystem no longer has a record of the file?

Seems reasonable.

Yeah, it's unclear what to do here if the BuildSystem might take some time to load it. Maybe settings(for:_:) can be shifted to be async (with a callback) to allow for this type of timeout?

Yeah, for settings(for:_:) it would be straightforward to timeout; I guess it's less obvious to me how we would coordinate that for a hypothetical design where we rely on getting an initial notification with settings from the delegate, since the delegate is more isolated from the query than a callback closure would be.

[DavidGoldman]

One option is for the BuildSystem to return some placeholder flags and then fix them up later (by sending a notification).

I'm wary of using fallback settings in the interim, because it would cause us to create a semantic AST in sourcekitd that is almost certain to have errors. So you're wasting resources typechecking an AST, and you end up with bad diagnostics like failing to import every module. I think you only want to use fallback settings when the build system doesn't know about the file. You could instead open the document with no settings so that you get syntactic support right away.

Potentially an optional, where null means the BuildSystem no longer has a record of the file?

Seems reasonable.

Yeah, it's unclear what to do here if the BuildSystem might take some time to load it. Maybe settings(for:_:) can be shifted to be async (with a callback) to allow for this type of timeout?

Yeah, for settings(for:_:) it would be straightforward to timeout; I guess it's less obvious to me how we would coordinate that for a hypothetical design where we rely on getting an initial notification with settings from the delegate, since the delegate is more isolated from the query than a callback closure would be.

As an alternative then, we could remove the settings(for:_:) and then let the build system notify the delegate when it has build settings for the requested files. In the meantime the LSP would likely ignore requests in a given file and defer opening until settings are known? Or default to empty settings (would likely have errors in both Objective-C and Swift)

It's also worth noting that at the moment the BuildSystemList always gives fallback build settings even if they may not be correct.

[benlangmuir]

Or default to empty settings (would likely have errors in both Objective-C and Swift)

Empty settings for swift means it will be syntax-only, so no semantic errors like module import failures. For clang I'm guessing clangd itself is going to choose fallback settings, but that's pretty hard to avoid.

[DavidGoldman]

Or default to empty settings (would likely have errors in both Objective-C and Swift)

Empty settings for swift means it will be syntax-only, so no semantic errors like module import failures. For clang I'm guessing clangd itself is going to choose fallback settings, but that's pretty hard to avoid.

So how does removing the settings(for:_:) and then letting the build system notify the delegate when it has build settings for the requested files sound? This will allow us to use empty arguments for Swift (but presumably have errors for C/C++/Objective-C), but I'm not sure how well it will work with the BuildSystemList due to the FallbackBuildSystem. If we want to support the BuildSystemList here we will probably have to modify things a bit.

Edit: or alternatively keep settings(for:_:) and keep it as a fast-path to return already loaded build settings?

[akyrtzi]

So how does removing the settings(for:_:) and then letting the build system notify the delegate when it has build settings for the requested files sound?

This is my preference, provide the settings along with the notification, which may or may not ever come, and let the LSP service deal with lack of settings however it wants.

If the build system can offer temporary or fallback settings, that is nice but not a requirement and it can do it by sending separate notifications.

[benlangmuir]

To test that the settings update is working correctly, I suggest using #if FOO and changing the compiler arguments to add/remove -DFOO.

[DavidGoldman]

Hmm this doesn't seem to be working, do I need to add in other Swift toolchain flags?

[benlangmuir]

I would expect it to work just like a command-line invocation of swiftc. E.g.

$ cat t.swift
#if FOO
func foo() {}
#endif

foo()

$ swiftc t.swift
t.swift:5:1: error: use of unresolved identifier 'foo'
foo()
^~~

$ swiftc t.swift -DFOO
# OK

[DavidGoldman]

With just the build settings of -DFOO I'm not seeing any diagnostics reported by SourceKit even with the above code

[benlangmuir]

In that example the diagnostics should be there when it is not defined.

[DavidGoldman]

I'm not seeing any diagnostics in the Swift code although it's working fine for ObjC

[benlangmuir]

Ah, sorry: you need to expect two notifications in Swift, because the syntactic errors (if any) come back immediately, but the semantic ones show up when the AST finishes typechecking/etc. You can see the way I've handled this in LocalSwiftTests.testEditing, but basically you just pass a second handler. I think you want to just have the first handler be empty, then do the check in the second one where you know you have semantic errors.

[DavidGoldman]

Interesting, I'm not seeing a second notification, presumably because SourceKit doesn't have the proper arguments for semantic analysis? I'll try mirroring the flags from the fallback build system.

[DavidGoldman]

Works with the FallbackBuildSystem

[benlangmuir]

@swift-ci please test

[benlangmuir]

So how does removing the settings(for:_:) and then letting the build system notify the delegate when it has build settings for the requested files sound?

This is my preference, provide the settings along with the notification, which may or may not ever come, and let the LSP service deal with lack of settings however it wants.

That seems fine for now, although I think we'll want to bring back a way to do a one-off request for settings for things like background indexing in the future. But we can deal with it then.

[akyrtzi]

That seems fine for now, although I think we'll want to bring back a way to do a one-off request for settings for things like background indexing in the future. But we can deal with it then.

If we make background indexing an operation supported by the build server then we don't need to get settings, we'll just request the build server to produce index data for certain files.

[benlangmuir]

Good point.

[DavidGoldman]

So how does removing the settings(for:_:) and then letting the build system notify the delegate when it has build settings for the requested files sound?

This is my preference, provide the settings along with the notification, which may or may not ever come, and let the LSP service deal with lack of settings however it wants.

If the build system can offer temporary or fallback settings, that is nice but not a requirement and it can do it by sending separate notifications.

Do you think it would make sense for the registerFor... call to return a FileBuildSettings?? e.g. so the BuildSystem can easily return FileBuildSettings if it has them for the initial openDocument call? Otherwise I guess the BuildSystem could immediately trigger a notification if it has the flags and the LSP could still use them for the initial open call.

[akyrtzi]

Do you think it would make sense for the registerFor... call to return a FileBuildSettings??

It's not clear to me there's particular benefit, it makes the interaction model a bit more complicated since the LSP will wait for both the register call to respond and for a possible notification. Since the LSP will wait for a response asynchronously anyway, we might as well make it so that it only waits for a notification, so the usage model will be a bit more straightforward.

[DavidGoldman]

Do you think it would make sense for the registerFor... call to return a FileBuildSettings??

It's not clear to me there's particular benefit, it makes the interaction model a bit more complicated since the LSP will wait for both the register call to respond and for a possible notification. Since the LSP will wait for a response asynchronously anyway, we might as well make it so that it only waits for a notification, so the usage model will be a bit more straightforward.

Ah, that makes sense if there's some sort of async communication protocol that we'd need to wait for. But if the BuildSystem is inside SourceKit-LSP / there's a synchronous channel to get them, you could potentially wait for the register call to return something and then trigger the open.

e.g.

No placeholder args
openDocument(/foo/main.c)
registerForChangeNotifications(/foo/main.c) --> settings
send clangd flags and then open

Placeholder/temp args
openDocument(/foo/main.c)
registerForChangeNotifications(/foo/main.c)
Send open to the clangd (uses temp flags causing errors)
fileBuildSettingsChanged([/foo/main.c])
Send clangd proper flags --> now is accurate

But I agree, we can always change this later on

[DavidGoldman]

Initial fixes, still need to:

• Remove settings(for:_:)
• Fix up the Swift BuildSystem test
• Add clangd BuildSystem test

[benlangmuir]

@swift-ci please test

[DavidGoldman]

Tests added, is it ok if I remove the settings(for:_:) in a follow up PR? Think that one will require modifying the behavior of the BuildSystemList a bit.

[benlangmuir]

@swift-ci please test

[benlangmuir]

@swift-ci please test

[benlangmuir]

@swift-ci please test

[DavidGoldman]

Ah looks like I need to merge with upstream to add support for the new BuildSystemBuildSystem from #151. Will do that tomorrow

[DavidGoldman]

OK, added support, tests should be good now

[DavidGoldman]

@swift-ci please test

[benlangmuir]

@swift-ci please test

[benlangmuir]

@swift-ci please test

[benlangmuir]

@swift-ci please test

[benlangmuir]

@swift-ci please test