[vm] dart2native on Windows fails to correctly compile nnbd opt-out code

[leafpetersen]

I'm seeing pkg/vm_snapshot_analysis/test/precompiler_trace_test breaking on the windows package bot only with the null safety flag flip CL. This test passes locally on my mac. See log here

I don't know whether this is an infra issue with the bots (cc @athomas ), or something with the test or the vm (cc @a-siva )

[leafpetersen]

Just a couple of notes on this. I confirmed that the test runs correctly on mac, and that the testSource code embedded in the test is run as opted out when run on mac. The error message on windows indicates that the testSource code embedded in the test is run as opted in + strong mode when run on windows.

[leafpetersen]

I'm not going to block the flag flip on this, since it seems to be something fairly idiosyncratic. I will plan on approving this failure, since this is the last failing test.

[mraleph]

This has been a long standing problem with this test on Windows which I think has roots in front-end: dart2native lib/main.dart interprets entry point as package URI on Mac and Linux but not on Windows for some reason. I have never been bothered enough to track this down (partially because my Windows machine is dead). /cc @johnniwinther

[franklinyow]

Since this is a long standing problem, and non-blocking, I'm moving this out of "flag flip work" Epic

[aam]

I think the issue is with dart2native canonicalizing source path, which results in different case for the filename compared to the package filename, which results in package_config inability to map filename to package uri and assuming that file have to be compiled using current sdk language version.

[leafpetersen]

cc @lrhn @natebosch is this something that package_config should handle?

[natebosch]

I think it would make the most sense to canonicalized paths as the file is written.

[lrhn]

Agree. I don't want package_config to start speculatively changing string capitalization. The package_config package is entirely based on URIs, not local paths, and it doesn't even try to check whether the specified paths actually exist. They don't have to exist, you could be building a package_config.json for deployment, so checking whether paths have the correct capitalization isn't really viable.

[aam]

@mit-mit

[mkustermann]

I've uploaded cl/170093 which fixes this test.

[ghost]

I think the issue is with dart2native canonicalizing source path, which results in different case for the filename compared to the package filename, which results in package_config inability to map filename to package uri and assuming that file have to be compiled using current sdk language version.

@aam, can you point me to where in package_config this happens?
I'm trying to understand the series of events you're describing. And what I'm not following is how the canonicalization of the source path being passed to gen_snapshot affects package_config's ability to match .. entries in the passed .packages file?

I would argue any of our tools that takes paths as command line input should be able to handle any valid path - whether it's been canon. or not.

[mkustermann]

The specific issue with dart2native should be fixed now and the mentioned test is passing now.

The problem occurred because a dart file path and a packages path were handled differently (former was lower cased, the latter not). Those paths are later converted into Uris, which then get used for package resolution. The .packages uses a ./ relative path (see here) - so this particular issue is not due to wrong casing inside the .packages file. The issue occurs when resolving ./ relative to the location of the .packages file itself (which had incorrect casing). Which causes the package:package_config to not detect that the given file (with lower cased directory names) is located inside the package (which had upper cased directory names) because it doesn't compare in case-insensitive way on windows (see here).

Now there's a higher level question:

• Where should the responsibility for case-insensitive handling of paths lie?
• Can the CFE assume all Uris given to it are canonicalized to lower-case letters?
  • If so, can CFE assume whoever generates package config files writes lower cased names as well?
  • If not, should we pass a isWindows boolean down to the CFE, which can use it internally as well as pass it down to package:package_config so both can do the proper canonicalization themselves?

If we want to move this responsibility to the place where paths get converted to Uris, then the right place might be in pkg/front_end/lib/src/fasta/resolve_input_uri.dart - @johnniwinther wdyt? (Other tools that don't use CFE's Path->Uri logic might need to be updated as well)

[aam]

The issue occurs when resolving ./ relative to the location of the .packages file itself (which had incorrect casing).

Why incorrect casing? Windows had its own concept of correct casing for files so for example resolving file name could result in mixed casing like "C:\Users\username\AppData\Local\Temp\instruction-sizes-test-collapse-by-packagef3c0a70e.packages".

Where should the responsibility for case-insensitive handling of paths lie?

I wonder whether it is worth an effort and complexity. Case-insensitivity of file paths is only applicable to Windows and normally doesn't get in a way if you get filename from the filesystem and just use it as is.

Separately it would also help to be clear to the user that their script was treated as nnbd opt-in vs opt-out code because it was not resolved to package uri.

[lrhn]

If someone writes dart c:\mycode\libs\floo\bin\floo.dart on the command line and the actual Windows directory is C:\MyCode\libs\floo\bin\floo.dart, then it matters what is written into the package_config.json for the floo package. If it says:

{
 "name": "floo",
 "rootUri": "file:///C:/MyCode/libs/floo/bin/floo.dart",
 ..

then a lookup of file:///c:/mycode/libs/floo/bin/floo.dart will not find it as belonging to the floo package.

On non-Windows platforms that behavior is correct because there might be both a MyCode and a mycode directory.
Since the checking happens at the URI level, and URIs are case sensitive and platform ignorant, we need to either canonicalize paths before turning them into URIs, or we need to accept that file names are not case insensitive even on Windows.

Alternatively, we can parameterize the package_config package with a bool isInside(Uri parent, Uri potentialChild) function which callers can override to do their own platform specific path comparison, and provide a Windows version of that.
It gets complicated fast.