Code completion sometimes shows stale results

[DanTup]

Here, notice name5 that is defined in the file on the right is missing from the completion list:

I can reproduce this by:

• Clearing the ~/.dartServer folder
• Open this project, including the (right) file myfoo.dart
• Add a new field to the class definition (name6)
• Save all files
• Restart VS Code
• Wait for analysis to complete
• Invoke completion

My theory is that this is related to using simple increasing integers as modified stamps (see here). When you open the file for the first time, the modification stamp is 0 or 1 (depending on the order of opened files), and this is embedded in cached data. When you reopen the project, even after the file was modified, the new instance of the server again starts from 0 and decides the cached data is still valid.

@bwilkerson @scheglov does this sound plausible? I can repro both on LSP and the original protocol.

[DanTup]

Changing overlayModificationStamp to start from millisecondsSinceEpoch` does seem to prevent it from happening:

/// The next modification stamp for a changed file in the [resourceProvider].
int overlayModificationStamp = DateTime.now().millisecondsSinceEpoch;

Presumably also using that value directly as the stamp (instead of starting from it and increasing) would also work. I'm not sure whether one is better than the other (or if there's something better still).

I tried to reproduce this in a test (by creating a second server so its variable would be reset to 0) without any luck (nor could I find where the modification stamp might be being used for the declarations).

[bwilkerson]

When you reopen the project, even after the file was modified, the new instance of the server again starts from 0 and decides the cached data is still valid. ... does this sound plausible?

I don't know. My understanding is that having the same modification stamp shouldn't be enough to cause a bad cache hit because I thought we were using a hash for the cache key that included more than the modification stamp and therefore ought to have been different. But that wouldn't explain why changing the starting value for the modification stamps would fix the problem. @scheglov Do you have any thoughts?

Presumably also using that value directly as the stamp ...

The only problem with that is a purely theoretic one and very unlikely to occur in practice. If two modification stamps are generated (either in server or by an external process) before the clock ticks over then we could theoretically end up using the same stamp for two different versions of the code. Using an monotonically increasing counter whose value is significantly different from the clock time should eliminate even the remote possibility of these occurring.

Out of curiosity, what happens if we start with a small but non-zero stamp value, such as 0xABCD? I'm just wondering whether significantly small values happen to not work well with the hash algorithm we're using.

[scheglov]

Yes, I think we have a bug in available declarations.

For performance reasons I attempted to use modificationStamp to avoid reading the file, and hashing it.
But we are using OverlayResourceProvider, and the way we set modification stamps makes them not reliable.
And in general, using only modification time is probably wrong, see https://apenwarr.ca/log/20181113 for details.

Starting overlayModificationStamp with a value that is time based will make it much less prone to such behavior, although theoretically will not remove it completely.

[DanTup]

Starting overlayModificationStamp with a value that is time based will make it much less prone to such behavior, although theoretically will not remove it completely.

Is there value in doing this (such as DateTime.now().millisecondsSinceEpoch) to reduce this until there's a better solution? I can make the change, although I failed to make a test that triggered this.

If I understand correctly, starting from milliseconds would require the user to modify more than one-file-per-millisecond on average in order to "overlap" the numbers with those used in a future session (or have multiple concurrent sessions with the same files being edited), so it would probably reduce the chance of it occurring to something very small.

[bwilkerson]

... require the user to modify more than one-file-per-millisecond on average ...

Correct. in production code the probability, while non-zero, is very low.

The place it's more likely to be seen is in tests, but they's have to be tests that restarted server, and as far as I'm aware we don't have any tests like that other than the benchmark tests (and those don't modify files).

I'm convinced. I think the product will be more stable if we make that change.

[scheglov]

I agree, doing this will make things better, and make the probability of issues very low.

[DanTup]

Fixed by dart-lang/sdk@85aecd5, thanks!