swift

[SourceKit] Update requests to handle locations within generated buffers

#64456

Merged

I don’t think there’s comment anywhere that describes the difference between PrimaryFile and InputFile. Could you add one somewhere? I wouldn’t mind if you repeated it in many places as I would be wondering what the difference is whenever I read code any of these functions.

I'll add a comment... somewhere 🤔

I added it to the functions that get primary_file/sourcefile

Thanks

Why do you use InputFile here but PrimaryFile almost everywhere else? I’m not saying this is wrong, I just don’t understand what drove the decision for InputFile vs PrimaryFile yet.

They should all be InputFile. I only see the one PrimaryFile in diagnostics. That one is a little confusing because diagnostics don't take an input file, only a primary file. Things are even more confusing because getFileSystem used to take a primaryFile and I've now changed what that actually means - we previously used "source file", "primary file", and "input file" interchangeably. I'll fix up all the callsites that currently have primaryFile= as their comment (since I renamed the arg).

InputFile == buffer we're doing the request on, could be a generated buffer name
PrimaryFile == file to build AST for
SourceFile == I'd prefer this isn't used, it can really mean either

I think I've changed my mind here. It doesn't really make sense to be able to provide a VFS for a generated input buffer name. So I'll change this back to use primary file.

OK, makes sense to me 👍🏽

What do you think about naming all the StringRefs PrimaryFileName and InputFileName. That creates a clear distinction between filenames and the files themselves.

Name is kind of weird to me since for a path on disk it implies the name rather than full path. I'd somewhat expect the contents itself to be called <whatever>FileContents. How strongly do you feel about this?

Not too strongly. It’s mostly that I expect PrimaryFile to be of type SourceFile * while reading the body of the function without the signature and not a StringRef. InputFilePath would be an alternative name. But if you have strong feelings, feel free to keep it as-is.

Path has similar issues, ie. it implies a path on disk when InputFile can actually just be the buffer name. These could be PrimaryFilePath and InputBufferName though, since the former is always a path and the latter is meant to be a buffer name/identifier, not a path/filename. Time for lots of renaming 😅

I renamed them all to PrimaryFilePath and InputBufferName. Hopefully that's more clear.

Did we previously set KeyPrimaryFile to an empty string if no primary file was passed?

We didn't set it at all. KeyPrimaryFile is new and I only added it for refactorings (see the removal in this file).

Since this is now returning the file name what do you think about renaming the function to getSourceFileNameForRequestOrEmitError?

See my other comment. Though given the LSP test failures on Linux (which are quite confusing to me), I'm actually going to change this to getInputFile which will return key.sourcefile only if it is different from key.primary_file and an empty string otherwise.