Make path in Document optional

[davidanthoff]

We currently seem to assume that we can compute a path for every document that VS Code asks us to open. That is not correct, we can get URIs with a non file scheme, and in those cases we cannot create a path from the URI.

This starts to fix this, but is not complete. I think there are various places in StaticLint that make an assumption that we can have a path always, right?

Do we maybe need a function like haspath that one can call on a doc, and then make sure that we handle the cases where that would return false?

PS: oh, this was triggered by this crash report.

[ZacLN]

From my recollection of writing this, .path is . stored because it's relatively costly to convert between URI and filepaths. For that reason I lean towards the haspath approach rather than removing the field.

Paths are used on the StaticLint side for naming of files (i.e. the FileServer stuff) - as far as I'm aware that doesn't place any requirements on the LanguageServerInstance to use them - e.g. there is no assumption on the existence of .path outside of StaticLint/src/server.jl - these functions are assumed to have their own equivalents in LanguageServer.

I hope that makes some sense

[davidanthoff]

Yep, that makes sense!

I reworked this PR so that we have a _path::Union{Nothing,String} field now for docs. I also removed a lot of the uri2filepath calls and used the precomputed path instead.

I tried to audit all calls to either uri2filepath or getpath and handle the situation where they return nothing. I assumed now that we can always convert workspace folder URIs, but not all URIs for files. I'm not 100% that is correct, but we can maybe handle the workspace folder stuff if it comes up in crash reports. Would be good if you could also audit things for this, I'm not sure I got all the situations.

And then I think you need to change StaticLint.jl to be able to deal with these nothing values for paths as well. I did see that we set the .val field of some datastructure to whatever uri2filepath returns, so I guess there we need the option to also store nothing, and then the code that deals with that needs to be hardened so that it can process nothing values, right? I'm not familiar enough with the StaticLint.jl code to make that change, I'm afraid :)

[davidanthoff]

@ZacLN do you think it will be tricky to adjust StaticLint.jl to this?

[ZacLN]

Is it possible to leave the _path field as a String (set to "" when there's nothing there and changing the === nothing comparisons to isempty)?

[davidanthoff]

I guess yes, but my thinking had been that by explicitly using nothing when there is no file we would more easily catch bugs if we forget to adjust some part of the code to special case handle this? Couldn't we easily end up in a situation where the empty string gets passed to some part of the code that really should special case the no-file situation, but then it just seems to work because it just treats an empty string as a file name?

[davidanthoff]

@ZacLN how do we proceed with this? Do you want me to change this to empty strings if there is no path, or stick with nothing (which seems much safer to me, TBH)? In any case, I think we should finish this as it fixes a bug we are seeing in crash reporting.

[ZacLN]

Can we use empty strings for now and revisit it later

[davidanthoff]

Alright, removed the Nothing stuff for now, ready to be reviewed!