Analysis server code reports errors on Windows

[DanTup]

It's possible this is related to #28895 and/or #32042 but today I was trying to get the analysis server running on Windows (so I can debug those issues), for the latest code (5ceaa66) I had 107 errors. They all seem to be "can't assign type x to x"-type errors, like this one:

The reason I'm not sure if this is the same as those others, is that in the message the capitalisation of the drive letter is different, and I think that might be causing it (in some of the others, the paths were cased the same).

I don't know if I'll be able to debug this without being able to run it, but I'll see what I can find.

[DanTup]

I tried to track this down to where the comparison of the types was done, but I ended up at code like this:

if (!_typeSystem.isAssignableTo(actualStaticType, expectedStaticType)) {

So it's possible the issue isn't in the comparison, but maybe at the point where the types are loaded from the files off disk. Could it be that there's nothing to handle differences in casing? I'll see if I can make a small repro to test this...

[DanTup]

I think that's it...

Although you could argue this one is user error; the analysis server code probably doesn't have anything that can be tweaked by the user - it could just be because sometimes Windows/apps will give you lowercase drive letters and sometimes they won't!

I don't think this is one for me, GL! (it's just a case of putting an if (os.caseInsensitivePaths) path = path.toLower() somewhere, right? 😉)

[devoncarew]

It looks like (from the first comment), the paths are M:\Coding\... and m:\Coding\...; identical expect for the case of the drive letter. It sounds like some normalization / comparison changes are necessary on windows. @bwilkerson and @scheglov would likely know the best way to go about this.

[DanTup]

I did have a stab at tracing through the code trying to understand where this might need doing, but got nowhere. I suspect the issue is in code that loads files from disk (and presumably before hand checks if they're already loaded - possibly be a case-sensitive path), but I'm not sure where that would be (I found a couple of places that seemed to be doing that, but hacking about at them didn't help).

I'm happy to take dig deeper if something can point in the right area though.

[bwilkerson]

In Dart, a single file can be loaded as two (or more) separate libraries if the URIs used to refer to the file are not equal. This normally occurs when users mix file: and a package: URIs.

However, URIs are care sensitive. So if we're getting different cases in URIs, then that would account for the issue.

Normalizing the paths (hopefully only the drive letter) would probably require the fewest changes to other code, and can probably be done in PhysicalResourceProvider. The potential downside of this is that we might report an error that uses a different case than what the user actually typed. I don't know how confusing users will find that. You probably have a better feeling than I do as to what Windows users would expect.

[DanTup]

Normalizing the paths (hopefully only the drive letter)

I don't know if just the drive letter will be enough (I've seen Windows APIs return the entire path lowercased intermittently on Windows before!) but it's probably a good start and should at least solve this one.

The potential downside of this is that we might report an error that uses a different case than what the user actually typed. I don't know how confusing users will find that

For the drive letter I don't think anyone will notice. If we lowercased the whole path, that might be annoying though (there's a years-old bug in Visual Studio where seemingly-randomly it'll show paths in tooltips entirely in lowercase and it winds me up!).

I could try and take a stab at something in PhysicalResourceProvider early next week.

[DanTup]

@bwilkerson

Ok, if I change the three calls to normalize with calls to canonicalize in that file, it does appear to fix the issue for the analyzer source (it's possible all three aren't required, haven't tested each). The dartdocs on those two methods suggest this might be the right thing?

This does not however fix the simple sample I posted above (importing with different cases), presumably because they're considered uris and are case-sensitive (for presumably the same reason, that code crashes at runtime with a TypeError too).

I'm not sure if you consider this a valid (or the best) fix. I'm finishing for the week now (I do my hours over Mon-Thur) but I can dig deeper early next week if you want.

[bwilkerson]

@scheglov Do you remember why we're using normalize rather than canonicalize?

[scheglov]

I don't think this was intentional decision.
Note that canonicalize effectively performs toLowerCase().

[DanTup]

Ok, I'm happy to do some more testing next week to see if changing all three is required and whether it impacts the error messages shown to the user.

[DanTup]

Seems like only the changes in getFile and getFolder are required, but it does seem to basically just lowercase the paths everywhere (including messages). It does feel a bit wonky:

Original:

A value of type 'Danny (M:\Coding\Applications\Google\dart-sdk\sdk\pkg\analyzer\lib\file_system\Danny.dart)' can't be assigned to a variable of type 'Danny (m:\Coding\Applications\Google\dart-sdk\sdk\pkg\analyzer\lib\file_system\danny.dart)'.

After change:

A value of type 'Danny (m:\coding\applications\google\dart-sdk\sdk\pkg\analyzer\lib\file_system\danny.dart)' can't be assigned to a variable of type 'Danny (m:\coding\applications\google\dart-sdk\sdk\pkg\analyzer\lib\file_system\danny.dart)'.

I guess the ideal would be that we preserve the path as we got it but used the canonical version only for comparisons - so all display to the user is nice but the casing differences don't break things. I don't know how involved that would be.

[bwilkerson]

That would be a fairly significant change. We also need to understand how the common front end is handling this case, and whether changes need to be made there. Then we can decide the best path forward.

[DanTup]

Ok, I guess this isn't something I can help progress right now then. I'm happy to help out if I can though since this issue prevents me from working on the analysis server from Windows (I'm trying to get both Mac + Windows set up for this but the MacBook I have doesn't work well connected to my big screen - VS Code is super blurry 😢)

[devoncarew]

Not sure of the full impact of this, but a custom canonicalizeAS() could just lower case the first path segment on windows. That would address the issue and minimize other possible impact.

[DanTup]

Good idea! I can take a shot at it and send a PR if it sounds good to @bwilkerson

[bwilkerson]

Yes, that sounds like a tractable amount of work, and while it won't solve all possible issues, it's a good first step. (And future steps might well be postponed until analyzer is on the front end.)

[DanTup]

I've raised a PR #32133 which fixes the analysis issues, though I can't run the tests locally atm (I don't think it's related - I'm working on it). I thought the PR might do a CI build to get real test results, but seems not. So, hang off merging the PR until I'm sure the tests are all still working!

[DanTup]

I was running the wrong tests here (analysis_server, but the change is in analyzer). Analyzer tests all pass both before/after this change:

02:46 +22943: All tests passed!

I need to do some digging into why the server tests don't work locally (it's unrelated to this change, they fail without it); the errors all look a bit random (lots of calling methods of null, for ex). Unless you think it's important to get them running now for this, I'll come back to them later so I can round off some of the other things I'm working on.

[DanTup]

I don't have powers to re-open this, but this fix has been reverted because of #32186. I'm gonna come back to it.

[DanTup]

Good news/bad news!

Good news: Windows tests are all fixed
Bad news: Reapplying the fix here which introduced #32186 does not cause any test failures! :-(

I will do some more digging...

[DanTup]

Ok, I've tracked down the problem... We're normalising the drive letter casing in the file system but there are other checks between the path that comes from the editor and things in memory. Specifically, this fails because in file_state.dart there's a function called hasUri that looks like this:

/**
 * Return `true` if there is a URI that can be resolved to the [path].
 *
 * When a file exists, but for the URI that corresponds to the file is
 * resolved to another file, e.g. a generated one in Bazel, Gn, etc, we
 * cannot analyze the original file.
 */
bool hasUri(String path) {
  bool flag = _hasUriForPath[path];
  if (flag == null) {
    File resource = _resourceProvider.getFile(path);
    Source fileSource = resource.createSource();
    Uri uri = _sourceFactory.restoreUri(fileSource);
    Source uriSource = _sourceFactory.forUri2(uri);
    flag = uriSource.fullName == path; // LOOK HERE
    _hasUriForPath[path] = flag;
  }
  return flag;
}

There's a string comparison on the paths here. I presume the reason for this is in the comment above the method (I don't fully understand it though).

So, normalising the drive letters in that equality check will fix the issue though I don't know if we're not starting to leak weird logic around the place. @bwilkerson @scheglov thoughts? (it's possible we'll be chasing issues in many places!?)

I should be able to build a failing test now, so I'll work on that too.

[bwilkerson]

Longer term I would definitely prefer that we converted file paths at the API and file system boundaries and didn't need to worry about it anywhere else. I don't know how hard that would be, but given that a lot of the API boundary code is generated, we might be able to do that fairly automatically by updating the generator.

[DanTup]

Ok, been doing more testing on this (I want to start on Signature Information and really need to be at 0 errors on this codebase before I start!).

If I apply my original changes (uppercasing the drive letter in physical_file_system.dart) the warnings all go away but all interactions with the server return no results. If I add canonicalize around that hasUri check above, it seems like all server interactions start working again (can't be certain everything does, but in a quick test it looks positive). @bwilkerson Are you happy adding canonicalize as a fix here? It's only on the comparison so I don't believe it should negatively affect anything (unless it lowercases things on case-sensitive platforms in which case it could cause other issues if users deliberately have overlapping filenames of different cases?!).

I'm going to attempt to write some tests that fail because of the original error (eg. without any of my changes they fail) and then some that fail after my changes (which fix the first test) and then hopefully they will pass if I make this canonical fix.

@bwilkerson If you want me to try and push this fix further up somewhere, I'm happy to do so, though I'm not sure how easy it'll be since there are a lot of calls to hasUri in different places and lots of server APIs that take in paths. Having tests should at least make it easier to test the options though.

[bwilkerson]

@DanTup Is this issue still relevant, or should we close it?

[DanTup]

The specific issue I was hitting here was caused by drive letters being cased wrong. In VS Code we now normalise drive letters to uppercase (VS Code normalises them to lowercase before giving to us!) so I can open the analysis server code without issues.

However, there is still the issue of paths being treated case-sensitively on Windows that we may wish to address long-term. It still comes up now and then, because for-ex on Windows if you cd myFOLDer in a terminal and then open Code (code .) you'll get paths with the casing as you changed into it, though to try and mitigate these I warn people if the casing we get from VS Code does not match the "real" casing on the file-system.

I think probably there should be an issue to track handling case-insensitivity better, but I don't know if it's covered by another one (or even what the fix will be, given case-sensitivity can vary by volume, and Dart lets you import files across volumes).

[asashour]

Just a small addition, this happens with also IntelliJ.

The error goes away when all references of package:some_package

import 'package:traveler/model/country.dart';

are changed to relative location, e.g.

import '../model/country.dart';

(or vice versa), but not the mix of relative and package:abc in different places

• Latest released IntelliJ/plugins
• Dart SDK version: 2.17.0-261.0.dev (dev) (Thu Mar 31 09:53:27 2022 -0700) on "windows_x64"

[asashour]

A test case can be found below, appreciate providing feedback, if this should be fixed at analyzer-server side, or IntelliJ should also unify the drive letter case when sending the requests.

If the former, should this be committed as failingTest?

Test case

analysis_server/test/integration/server/windows_test.dart

// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:async';
import 'dart:io' show Platform;

import 'package:analysis_server/protocol/protocol_generated.dart';
import 'package:path/path.dart';
import 'package:test/expect.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';

import '../support/integration_tests.dart';

void main() {
  defineReflectiveSuite(() {
    defineReflectiveTests(WindowsTest);
  });
}

@reflectiveTest
class WindowsTest extends AbstractAnalysisServerIntegrationTest {
  Future<void> test_driver_letter() async {
    if (!Platform.isWindows) return;

    var pathnameA = sourcePath('lib/a/a.dart');
    var pathnameB = sourcePath('lib/b/b.dart');
    var pathnameC = sourcePath('lib/c/c.dart');

    if (!isAbsolute(pathnameB)) return;

    writeFile(pathnameA, '''
class A {}
''');

    writeFile(pathnameB, '''
import 'package:test/c/c.dart';

import '../a/a.dart';

class B {
  void m() {
    C(A());
  }
}
''');

    writeFile(pathnameC, '''
import 'package:test/a/a.dart';

class C {
  A a;
  C(this.a);
}
''');

    standardAnalysisSetup();

    var lowerCase =
        pathnameB.substring(0, 1).toLowerCase() + pathnameB.substring(1);

    sendAnalysisSetSubscriptions({
      AnalysisService.OVERRIDES: [lowerCase],
    });

    await analysisFinished;

    expect(currentAnalysisErrors[pathnameB], isEmpty);
    expect(currentAnalysisErrors[lowerCase], isNull);
  }
}

[DanTup]

appreciate providing feedback, if this should be fixed at analyzer-server side, or IntelliJ should also unify the drive letter case when sending the requests.

I think the ideal (or even correct) fix would be in the server, but I think it's going to be quite complicated. There are some questions that might need answering/defining. For example, if the server just internally normalises the drive letter to uppercase on all inbound requests, is it valid for it to send uppercase drive letters back in responses when the client sent lowercase drive letters?

Eg.:

• client sends setAnalysisRoots ["c:\foo"] // lowercase drive letter
• server sends back diagnostics for C:\foo\bar.dart // uppercase drive letter

My feeling is that clients should handle this, but I'm not certain that they currently do (I'm certain I have some caches in VS Code that accidentally use the path as a case-sensitive key).

VS Code's current response to questions about case handling (for paths in general, not just drive letters) seems to be "everyone should handle the casing as the filesystem does", so any language server should be prepared to get a mismatch of drive letter casing and just handle with it.

Another question is whether any fix here should apply only to drive letters, or whole paths. VS Code's current response to questions about path casing is that all servers should just handle things the way the user expects (that is, mismatched cases should be treated the same on a case-insensitive volume). To complicate things though, they're not actually doing this in VS Code, they're just assuming all paths are case-insenstive on macOS and Windows, and all paths are case-sensitive on Linux (if you break these assumptions, weird things can happen - microsoft/vscode#94307).

To complicate things further, if you rename a file by only its case in VS Code, it will forever continue to use the old casing in the API (eg. it will tell the server you renamed fileA to file_a but then continue to use fileA in all requests, even after restarting VS Code - see microsoft/vscode#121106). This means Dart users often see spurious file_name lint violations after they've fixed the filename issue, as the request to the server creates an overlay using that old original casing. As a workaround, I have a setting in Dart-Code that will normalise paths to how they appear naturally on the filesystem on every request (a rather expensive operation). The real fix for this should also be in the server, but a fix for this is likely a much larger job than just drive letter normalisation.

[asashour]

I also believe the correct way is from the server side, considering the current restrictions of the IDEs, and I assume the effort would not be great, because we have 2 IDEs with possible three OSs. The solution may not be that the server always normalizes the driver letter, it can be: send whatever makes the client-side (and user) happy (if we can't change the client behavior).

The main concern I see, is that even with a specific IDE in a specific platform, it sends requests with different case-sensitive drive-letter (e.g. setAnalysisRoots in upper-case, while setPriorityFiles in lower-case). Now a question from the server side, how would we respond? A potential answer could be, we need to test the current behavior of the IDEs.

An additional difficulty is that this issue occurs with me in only a single project

Regarding the driver-letter or the paths, these are possibly different things, let's tackle them one by one.

We have access to the server, plugins, and IDE teams, so it would be hopefully possible to fix the issue at the end.

[DanTup]

Now a question from the server side, how would we respond? A potential answer could be, we need to test the current behavior of the IDEs.

My understanding is that VS Code (the editor)'s intended behaviour is to handle this fine. I'm not certain all places in the Dart extension will handle this correctly, but I think any issues resulting from getting back different drive letters should be fairly easy to fix (paths, less so). I don't know about the other editors though.

[bwilkerson]

Is this still an issue?

[DanTup]

The original problem is not an issue because the drive letter (which is where the casing differences came from) is always normalised in the Dart-Code extension.

Theoretically, other clients could bump into this if they did not normalize the drive letters (or otherwise sent mixed-case paths). Having the server handle case-insensitive paths better would be nice, but last time I tried to do this it seemed complicated (there are lots of places where paths are used as map keys and in comparisons) so I don't know if it's worthwhile unless we know it's causing issues anywhere.

(so my feeling is this could be closed until we know it's causing a problem, but I'll leave that decision to you)

[bwilkerson]

Sounds good to me.