Analyzer reports mismatched types on Windows due to drive letter casing

[DanTup]

Copying this from Dart-Code/Dart-Code#261 as I don't think it's specific to Dart Code (however, I can't explain why Atom or DartAnalyzer wouldn't show the same issue).

Here is a smaller repro than attached to the Dart Code issue, it's basically:

lib/a.dart

import 'package:repro/b.dart';

class Renderer
{

}

class Mesh extends Renderable
{
    @override
    void render(Renderer r)
    {

    }
}

lib/b.dart

import 'package:repro/a.dart';

abstract class Renderable
{
    void render(Renderer r);
}

There's also a pubspec with just a name and I've run pub get.

The error reported is:

file: 'file:///c%3A/Users/danny/Desktop/repro/repro/lib/a.dart'
severity: 'Error'
message: 'Invalid override. The type of 'Mesh.render' ('(Renderer) → void') isn't a subtype of 'Renderable.render' ('(Renderer) → void').'
at: '11,5'
source: 'dart'

There's only one Renderer class so it seems like these should be the same and the error is incorrect, however the files reference each other, I'm not sure if that's supported?

The log from the AS server is this:

[11:47:34]: ==> {"id":"1","method":"server.setSubscriptions","params":{"subscriptions":["STATUS"]}}
[11:47:34]: ==> {"id":"2","method":"analysis.setAnalysisRoots","params":{"included":["c:\\Users\\danny\\Desktop\\repro\\repro"],"excluded":[]}}
[11:47:34]: <== {"event":"server.connected","params":{"version":"1.17.0","pid":12036,"sessionId":""}}
[11:47:34]: <== {"id":"1"}
[11:47:34]: <== {"event":"server.status","params":{"pub":{"isListingPackageDirs":true}}}
[11:47:34]: <== {"event":"server.status","params":{"pub":{"isListingPackageDirs":false}}}
[11:47:35]: <== {"id":"2"}
[11:47:35]: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":true}}}
[11:47:35]: <== {"event":"analysis.errors","params":{"file":"c:\\Users\\danny\\Desktop\\repro\\repro\\lib\\a.dart","errors":[]}}
[11:47:35]: <== {"event":"analysis.errors","params":{"file":"c:\\Users\\danny\\Desktop\\repro\\repro\\lib\\a.dart","errors":[{"severity":"ERROR","type":"COMPILE_TIME_ERROR","location":{"file":"c:\\Users\\danny\\Desktop\\repro\\repro\\lib\\a.dart","offset":113,"length":39,"startLine":11,"startColumn":5},"message":"Invalid override. The type of 'Mesh.render' ('(Renderer) → void') isn't a subtype of 'Renderable.render' ('(Renderer) → void').","code":"strong_mode_invalid_method_override","hasFix":false}]}}
[11:47:35]: <== {"event":"analysis.errors","params":{"file":"c:\\Users\\danny\\Desktop\\repro\\repro\\lib\\a.dart","errors":[{"severity":"ERROR","type":"COMPILE_TIME_ERROR","location":{"file":"c:\\Users\\danny\\Desktop\\repro\\repro\\lib\\a.dart","offset":113,"length":39,"startLine":11,"startColumn":5},"message":"Invalid override. The type of 'Mesh.render' ('(Renderer) → void') isn't a subtype of 'Renderable.render' ('(Renderer) → void').","code":"strong_mode_invalid_method_override","hasFix":false}]}}
[11:47:35]: <== {"event":"analysis.errors","params":{"file":"c:\\Users\\danny\\Desktop\\repro\\repro\\lib\\b.dart","errors":[]}}
[11:47:35]: <== {"event":"analysis.errors","params":{"file":"c:\\Users\\danny\\Desktop\\repro\\repro\\lib\\b.dart","errors":[]}}
[11:47:35]: <== {"event":"analysis.errors","params":{"file":"c:\\Users\\danny\\Desktop\\repro\\repro\\lib\\b.dart","errors":[]}}
[11:47:35]: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":false}}}

[lrhn]

The problem seems to be how you refer to your entry-point and the relation between the libraries.

If you refer to a.dart it as file:///..../a.dart (which you do, it says file: 'file:///c%3A/Users/danny/Desktop/repro/repro/lib/a.dart'), then it will be considered a different library than the one with URI package:repro/a.dart. That means that the Renderer in the entry-point library is different from the one imported by b.dart. You have two different versions of the same library. If you had given the library a name, you would get a warning here.

So either run/analyze using a package URI or stop doing absolute imports. I recommend the latter in any case. It's an unnecessary complication to hard-code your own package name into internal references, at least unless absolutely necessary. If nothing else, it makes it harder to rename the package, and it doesn't do anything good.

That is, either run/analyze it as dart package:repro/a.dart or try changing the imports from being absolute package-URIs to being relative: import "a.dart"; and import "b.dart";.

In the linked Dart-Code example, you say that changing the package-URIs work, and it is a solution.

As for Dart-Code, they might want to refer to files that are part of a package by its package-URI, not its file-location, if that is possible.

[DanTup]

@lrhn

The problem seems to be how you refer to your entry-point and the relation between the libraries.

If you refer to a.dart it as file:///..../a.dart (which you do, it says file: 'file:///c%3A/Users/danny/Desktop/repro/repro/lib/a.dart'),

That filename is what comes back from the analysis server and then gets passed to VS Code in the errors list; nowhere in the repro is importing it like this (nor do we pass it to the analyzer; only the workspace root). That path is (presumably) constructed by the analysis service based on the analysis root we pass to it; I don't believe there's anything I can change here to fix this.

As for Dart-Code, they might want to refer to files that are part of a package by its package-URI, not its file-location, if that is possible.

Dart Code doesn't invoke the analyzer directly; it spawns the analysis server from the snapshot in the SDK and then passes it the root folder. We don't parse any of the code at all. We just say "here's a folder of Dart code" and the analysis server gives us all the errors; how pubspec/lib/imports are handled are all internal to the analysis service. I believe this issue could be repro'd entirely without Dart Code (though I'm surprised it apparently doesn't occur in Atom, which uses the same service; though I don't have Atom to test it myself).

It's possible this code is incorrect and should be using relative paths; though the author expected it to work (and at first glance, I would to; though I'm not all that experience with Dart). I'm struggling to see how I can fix this in Dart Code though :(

[lrhn]

That sounds like it's probably an analyzer problem then.

If the analyzer sees the pubspec.yaml with name: repro and a lib/a.dart file, it should probably treat a.dart as having URI package:repro/a.dart when analyzing it. If it doesn't, problems like this will occur.
I'm tagging this as an analyzer issue.

The error report does need to show the file name - the analysis server can handle large directories with multiple package structures, so a package URI doesn't uniquely identify the file with the problem.

[bwilkerson]

If the analyzer sees the pubspec.yaml with name: repro and a lib/a.dart file, it should probably treat a.dart as having URI package:repro/a.dart when analyzing it. If it doesn't, problems like this will occur.

Yes, that's exactly what analysis server does. And as a consequence, we advise people to always use package: URIs when referring to libraries inside the lib directory of a package.

[bwilkerson]

I can't reproduce this behavior on bleeding edge. I created a directory structure containing the following:

repro
  lib
    a.dart
    b.dart
  pubspec.yaml

where a.dart and b.dart are identical to what's above, and pubspec.yaml contains

name: repro
version: 0.0.1
description: A test package.

I then ran pub get to produce a .packages file and a pubspec.lock file.

When I run the command-line analyzer on a.dart I get "No issues found!", and when I load repro in IntelliJ I see no errors being reported.

Could you enable the instrumentation log file, load a project like this into Dart Code, and then send me the log file? (Private e-mail is fine if you don't want to attach it.) That will let me see whether there's a mismatch between client and server.

[DanTup]

Yes, that's exactly what analysis server does.

Yeah, I figured this most generally work or there would be loads of issues!

Could you enable the instrumentation log file, load a project like this into Dart Code, and then send me the log file?

Yep, attached is the log for the zip file I attached at the top.

analyzer.instrum.txt

I'm puzzled about why it only seems to happen in Dart Code; it doesn't seem like Dart Code is doing much that could influence this, but the original reporter also tested in Atom and it didn't happen!

[bwilkerson]

Yeah, I'm puzzled too.

Unfortunately, the instrumentation log doesn't show anything that gives me any clues. There are only two requests, both perfectly reasonable, and there's nothing else that Dart Code needed to do. It might be interesting to get a similar log from Atom for comparison, but I honestly don't expect to see anything different.

Just for completeness...

Do you and the user both have a .packages file in repro?

Do either you, or the user, have any files or directories in either repro or any parent directory with any of the following names: .analysis_options, analysis_options.yaml, WORKSPACE, READONLY, or .jiri_root? If so, which?

[DanTup]

Do you and the user both have a .packages file in repro?

Yep; it was in the zip file up top, but here's the contents:

# Generated by pub on 2017-02-25 11:43:46.036993.
repro:lib/

Do either you, or the user, have any files or directories in either repro or any parent directory with any of the following names: .analysis_options, analysis_options.yaml, WORKSPACE, READONLY, or .jiri_root? If so, which?

I don't believe so, but just to be sure I moved the repro folder to the root of a drive (m:\repro) and tried again and see the same behaviour upon opening:

[19:48:51]: ==> {"id":"1","method":"server.setSubscriptions","params":{"subscriptions":["STATUS"]}}
[19:48:51]: ==> {"id":"2","method":"analysis.setAnalysisRoots","params":{"included":["m:\\repro"],"excluded":[]}}
[19:48:51]: <== {"event":"server.connected","params":{"version":"1.17.0","pid":3368,"sessionId":""}}
[19:48:51]: <== {"id":"1"}
[19:48:51]: <== {"event":"server.status","params":{"pub":{"isListingPackageDirs":true}}}
[19:48:51]: <== {"event":"server.status","params":{"pub":{"isListingPackageDirs":false}}}
[19:48:52]: <== {"id":"2"}
[19:48:52]: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":true}}}
[19:48:52]: <== {"event":"analysis.errors","params":{"file":"m:\\repro\\lib\\a.dart","errors":[]}}
[19:48:52]: <== {"event":"analysis.errors","params":{"file":"m:\\repro\\lib\\a.dart","errors":[{"severity":"ERROR","type":"COMPILE_TIME_ERROR","location":{"file":"m:\\repro\\lib\\a.dart","offset":113,"length":39,"startLine":11,"startColumn":5},"message":"Invalid override. The type of 'Mesh.render' ('(Renderer) → void') isn't a subtype of 'Renderable.render' ('(Renderer) → void').","code":"strong_mode_invalid_method_override","hasFix":false}]}}
[19:48:52]: <== {"event":"analysis.errors","params":{"file":"m:\\repro\\lib\\a.dart","errors":[{"severity":"ERROR","type":"COMPILE_TIME_ERROR","location":{"file":"m:\\repro\\lib\\a.dart","offset":113,"length":39,"startLine":11,"startColumn":5},"message":"Invalid override. The type of 'Mesh.render' ('(Renderer) → void') isn't a subtype of 'Renderable.render' ('(Renderer) → void').","code":"strong_mode_invalid_method_override","hasFix":false}]}}
[19:48:52]: <== {"event":"analysis.errors","params":{"file":"m:\\repro\\lib\\b.dart","errors":[]}}
[19:48:52]: <== {"event":"analysis.errors","params":{"file":"m:\\repro\\lib\\b.dart","errors":[]}}
[19:48:52]: <== {"event":"analysis.errors","params":{"file":"m:\\repro\\lib\\b.dart","errors":[]}}
[19:48:52]: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":false}}}

I can't reproduce this behavior on bleeding edge

Oh crap, I just realised when looking at the files that I totally neglected to mention that strong-mode is enabled via .analysis_options:

analyzer:
  strong-mode: true

It was in the zip, but I didn't mention it in the text so if you tried building your own repro you probably missed this.

I'm not sure how to enable logging in Atom; but if you think the log would be useful to compare, I'm happy to install it and try and figure it out. Since I haven't repro'd it myself I can't say for sure the OP didn't do something differently and maybe it can be repro'd there; so the "only happening in Dart Code" thing might be a red herring.

[bwilkerson]

I enabled strong-mode, but as I expected there was no difference.

[DanTup]

Could it be OS-related? I don't have easy access to a non-Windows machine to test, though maybe you have access to a Windows one?

Using the zip file I attached is probably easiest in case I described anything else incorrectly; just opening that folder in Dart Code immediately triggers the error.

If there's anything else you can think of that I can do to help debug, let me know!

[DanTup]

I opened the same repro folder on both my PC and Mac, both using 1.24.2 SDK. The PC reports this error and the Mac does not.

So I wonder whether there's some code that considers both the package import and the relative path as the same file that isn't working correctly on Windows (maybe because of path separators?).

[bwilkerson]

If it is a Windows specific issue that would explain why I couldn't reproduce it on my Mac.

So I wonder whether there's some code that considers both the package import and the relative path as the same file that isn't working correctly on Windows (maybe because of path separators?).

That sounds sort of plausible, but I'm not sure where that would happen. Analyzer resolves URIs against URIs and then uses a platform-aware algorithm to convert the resulting URI to a file path. If there isn't something interesting about this case, I would have expected a lot more reports of problems from Windows users.

[DanTup]

I thought that too. Do any of you have access to debug this on a Windows machine? I don't have the code running on mine currently, but it probably wouldn't be hard to sort out if someone could give me some pointers on how to debug, I could see if I can get anywhere.

[DanTup]

This appears to be fixed by #32133 (normalisation of drive letters on windows, the fix for #32095).

[bwilkerson]

Do any of you have access to debug this on a Windows machine?

Unfortunately, no.

[DanTup]

@bwilkerson That was a pretty old comment you replied to :) I believe this issue is fixed along with the other similar issues I sent a PR for.

[bwilkerson]

Yes :-) but the info about our lack of access to a Windows machine isn't likely to change soon.

[DanTup]

I don't think it's a big issue for this case, if the PR I sent is good then I think this is resolved. Are there CI builds/test runs on Windows to ensure I didn't totally smeg everything?

I don't mind digging into Windows-specific issues where I can; I tend to be quite split across Windows and Mac lately anyway.

[bwilkerson]

Yes, we have bots that run everything on all three platforms: Linux, Mac, and Windows. But the windows bots are not represented in the subset of try bots, so you won't know you've broken windows until the build fails.

[DanTup]

Good to know, ta!

[DanTup]

Re-opening this since the fix mentioned above was reverted and this might be a much easier case to address/fix than #32095 so it'd make sense to get out of the way first.

[DanTup]

@devoncarew @bwilkerson This seems to be cropping up much more recently. I'm happy to help out if I can - previously I tried to create some Windows tests that failed for the same reasons, but got stuck because the memory file system used in tests is effectively case-sensitive. If anyone has ideas about the best way to fix that, I don't mind having a go.

(though I'm still slightly blocked by #32702... have managed to get down to the command that's failing, but not figured out why!)

[escamoteur]

I'm too are now affected. You can try it with https://github.com/escamoteur/making_flutter_reactive-/tree/problem_with_analyzer

when running the App or running flutter analyze everything is ok but in VS code I shows two errors

In homepage.dart

[devoncarew]

@DanTup, it sounds like the fix was correct was correct, but we saw some test failures associated with it. Can you in-line the test failures, or reference them here? I think we should look at addressing them and re-applying the fix.

[escamoteur]

I just found the reason, it's because I used a relative path

import '../service/weather_entry.dart';

if I change that to package it's gone

[DanTup]

@devoncarew We don't have tests to cover this; when we made the original fix we shipped it because everything passed (we subsequently discovered that some Windows tests were being skipped, but even when running them, none of the failures relate to this).

I'd like to make some failing tests for this (and for the issues introduced by the "fix" we rolled back), but both of them rely on being able to write tests where the file-system is case-insensitive (which isn't the case for the unit tests) or integration tests where we can ensure the VM and whatever's driving the analysis server are using different cased drive letters.

I'll add to the list to discuss tomorrow and maybe we can come up with a plan.

@escamoteur Mixing relative and package: imports definitely makes things worse, but I don't think all of the issues are caused by that so you may still hit some issues.

[bwilkerson]

I don't think there would be a problem with making MemoryResourceProvider (and it's associated classes) consistent with the platform (such as being case sensitive when the underlying platform is case sensitive). That seems to me like the easiest path forward.

[DanTup]

That seems to me like the easiest path forward.

I thought that'd be an easy change to make, but I think it was full of Maps to look things up, so it may mean replacing all of those with something we can have a case-insensitive key for?

Also, to confuse things, new Macs seem to also be case-insensitive (though don't hit this issue generally because there isn't a random part of the path being exposed with differing casing in different places) so I'm not sure how complicated this will get (for ex. if I have both a case-sensitive and case-insensitive filesystem on my Mac and run Dart programs from both, should they honour the sensitivity for each drive? Could be a can of worms!)

[bwilkerson]

... it may mean replacing all of those with something we can have a case-insensitive key for?

One of the constructors for LinkedHashMap allows an clients to override the equality and hash functions that it uses, so that ought to solve that problem.

... new Macs seem to also be case-insensitive ...

Hm. I'm running 10.13.3, and at least in a console window that appears to be true. All of our tests are passing, though. Raises the interesting question of whether case insensitivity is really the issue here.

... should they honour the sensitivity for each drive?

I would avoid that if at all possible.

[DanTup]

Hm. I'm running 10.13.3, and at least in a console window that appears to be true. All of our tests are passing, though. Raises the interesting question of whether case insensitivity is really the issue here.

I think it's just luck that on Mac all the paths that come from the system are consistent, but on Windows the drive letters are not. Does make me wonder whether we can repro the issue by faking the casing of the path in setAnalysisRoots to something different to what the underlying folders were cased though. I'll have a quick play tomorrow and see where I get.

[DanTup]

Ok, I repro'd on a Mac really easily (not sure why I hadn't tried this sooner!).. Took the zip file from the very top of this post, and I made this in Dart Code where we send analysis roots:

newRoots[0] = newRoots[0].toUpperCase();

This results in the path being used by the editor to not match the one that the analysis server is using for package URIs, and we get the original error:

{
	"resource": "/USERS/DANTUP/DESKTOP/REPRO/lib/a.dart",
	"owner": "dart",
	"code": "strong_mode_invalid_method_override",
	"severity": 8,
	"message": "Invalid override. The type of 'Mesh.render' ('(Renderer) → void') isn't a subtype of 'Renderable.render' ('(Renderer) → void').",
	"source": "dart",
	"startLineNumber": 11,
	"startColumn": 5,
	"endLineNumber": 11,
	"endColumn": 44
}

So, if we have integration tests that use the real file system (and not the in-memory one), a failing test should be pretty easy. If not, we might still need to change the in-memory file system to be case-insensitive to do that.

Fixing things is its own can of worms. What casing of paths do we send back to the client - the case they sent to server or the case we think it is? And how do we handle mixed-case-sensitivity (you can import a Dart file from another drive)?

We could try a partial fix similar to what we did last time, which is to just normalise Windows drive letters in one direction everywhere and inform clients they have to deal with that - that all drive letters out of the server will be upper(/lower)case regardless of what they sent (or even, that they must always send uppercase).

I'm now wondering if me normalising drive letters to uppercase in Dart Code would actually fix this (well, at least for the Windows users hitting it casually) - VS Code currently normalises to lowercase (for the editor, sadly it's uppercase for the debugger!) if the SDK is consistently getting uppercase drive letters (it's hard to say without knowing where it gets that from). If this turns out to be a huge job, I can give it a go - I think that in all cases where VS Code is normalising my drive letter I'll be accessing the fsPath property, so it may be an easy job to map them all (and possibly even have a validation script or something to check for this).

[DanTup]

I'm now wondering if me normalising drive letters to uppercase in Dart Code would actually fix this (well, at least for the Windows users hitting it casually)

I don't know how risky this is (for ex. if there are any people currently getting lowercase drive letters in the analysis server, this will break things for them) but I've put the changes on a branch behind a flag and given a custom build to @escamoteur. If all goes well, this may be a quicker way to solve the users issues without having to divert a load of effort here.

[DanTup]

Slight update on this... I shipped a flag in the last version of Dart Code that allows users to force the drive letters to uppercase. It fixes some of the issues, but others remain relating to casing of other parts of the parts.

I found I can easily repro this on my Mac without needing to make changes to Dart Code (it's possible it'll repro in IntelliJ in the same way). Open the original repro from the very top of this Issue, but provide an incorrectly cased path to Code, for example, I extracted it to a folder named repro but then opened it like this:

code ~/Desktop/REPRO

This causes Code to think the folder is cased as REPRO and we send this to the analysis server. Since the server uses the correct case for all of the package: imports, we get the original error:

Invalid override. The type of 'Mesh.render' ('(Renderer) → void') isn't a subtype of 'Renderable.render' ('(Renderer) → void').

I'm going to see if I can detect when this happens (if I can get the "real" path of the workspace at startup, I can compare it to what's opened) and show a warning.