Dart imports are not case sensitive

AbhishekDoshi26 commented 2 years ago

Summary of Issue:

Recently encountered an issue which depicts that flutter's imports are not case sensitive. So for Flutter Engine: import 'package:test_project/screens/home.dart'; and 'package:test_project/Screens/home.dart'; are same which is totally incorrect. Now if the user imports as 2nd option, other services like firebase Hosting, Docker, GitHub actions, fail as they all are case sensitive and for them, this import will be undefined.

In one sentence, my folder name is screen and I imported it with Screen and still it works without any issues!

Steps to Reproduce

Create a new project with main.dart as the basic file
Create another folder called screens and inside that create a file called home.dart
Import the file in main.dart and change the import to have it Screen\home.dart instead of screen\home.dart

Expected results: Dart Analyzer should throw an error for import not found as Screen and screen should be 2 different folders.

Actual results: It works! No errors, no warnings!

Found on: macOS 12.2.1, Flutter version 2.8.1 as well as 2.10.2, Dart version 2.15.1 as well as 2.16.1

Code sample

#### main.dart ``` import 'package:flutter/material.dart'; import 'package:test_project/Screens/home.dart'; void main() => runApp(const MyApp()); class MyApp extends StatelessWidget { const MyApp({Key? key}) : super(key: key); @override Widget build(BuildContext context) { return const MaterialApp( title: 'Material App', home: Home(), ); } } ``` #### home.dart ``` import 'package:flutter/material.dart'; class Home extends StatelessWidget { const Home({Key? key}) : super(key: key); @override Widget build(BuildContext context) { return Container(); } } ```

Logs

#### flutter run ``` Launching lib/main.dart on Chrome in debug mode... Waiting for connection from debug service on Chrome... 12.5s This app is linked to the debug service: ws://127.0.0.1:60085/QgzYCoQNVzo=/ws Debug service listening on ws://127.0.0.1:60085/QgzYCoQNVzo=/ws 💪 Running with sound null safety 💪 🔥 To hot restart changes while running, press "r" or "R". For a more detailed help message, press "h". To quit, press "q". An Observatory debugger and profiler on Chrome is available at: http://127.0.0.1:60085/QgzYCoQNVzo= The Flutter DevTools debugger and profiler on Chrome is available at: http://127.0.0.1:9104?uri=http://127.0.0.1:60085/QgzYCoQNVzo= ``` #### flutter analyze ``` Analyzing test_project... No issues found! (ran in 1.5s) ``` #### flutter doctor -v ``` [✓] Flutter (Channel stable, 2.8.1, on macOS 12.2.1 21D62 darwin-arm, locale en-GB) • Flutter version 2.8.1 at /Users/abhishekdoshi/Desktop/flutter • Upstream repository https://github.com/flutter/flutter.git • Framework revision 77d935af4d (10 weeks ago), 2021-12-16 08:37:33 -0800 • Engine revision 890a5fca2e • Dart version 2.15.1 [✓] Android toolchain - develop for Android devices (Android SDK version 31.0.0) • Android SDK at /Users/abhishekdoshi/Library/Android/sdk • Platform android-31, build-tools 31.0.0 • Java binary at: /Applications/Android Studio.app/Contents/jre/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7249189) • All Android licenses accepted. [✓] Xcode - develop for iOS and macOS (Xcode 13.2.1) • Xcode at /Applications/Xcode.app/Contents/Developer • CocoaPods version 1.11.2 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] Android Studio (version 2020.3) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7249189) [✓] VS Code (version 1.64.2) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.34.0 [✓] Connected device (1 available) • Chrome (web) • chrome • web-javascript • Google Chrome 98.0.4758.102 • No issues found! ```

lrhn commented 2 years ago

Actually, imports are case sensitive. That itself is a well known issue.

Dart imports are URIs, and are considered the same if the two URIs are completely identical (after some URI nomalization).

URIs are case sensitive, so the two URIs here are different, which means that the Dart compiler treats them as different libraries if both occur in the same program.

When loaded from the underlying file system, if that file system is not case sensitive (like Windows), then it will load two different libraries which happen to contain declarations with the same names. Much hilarity ensues (like "Foo is not a Foo" errors.)

On a case-sensitive file system (Unixes), the two URIs will point to different files, which may both exist if you really have both a screens and a Screens subdirectory in the same directory. (That is obviously going to be a problem if you ever want to check that code out on a Windows computer, so it's really not recommended.)

I think MacOS can be configured to be either case sensitive or not (from a very quick search, I don't have a Mac available).

That means that this is probably working as intended. Your computer's file system reports that test_project/lib/Screens/home.dart exists, and it allows the compiler to load it successfully. Everybody's happy, no-one involved can see any problem. Windows users hit this a lot.

You might be able to change the file system to be case-sensitive (although I think it requires reformatting, so back up first, reformat, and the restore, which is likely tedious, but doable).

We probably should warn people against having upper-case letters in paths at all, just as a style lint (one that you can ignore by putting, say, // ignore: lower_case_paths before the import.)

AbhishekDoshi26 commented 2 years ago

I totally agree with your answer @lrhn , that dart will follow what OS follows. It would be good if we have a lint rule which warns this.

Because if we don’t have it, probably many pipelines fail and subsequently break the production.

srawlins commented 2 years ago

@bwilkerson this sounds like a Hint candidate (sorry we keep having the distinction).

In one sentence, my folder name is screen and I imported it with Screen and still it works without any issues!

I think the lint rule here would then report when you import a path which doesn't exist, considering case. I wonder why we don't do this yet...

bwilkerson commented 2 years ago

While it might be worthwhile to have a lint to enforce a preferred style, I'd be hesitant about making it a hint (that is, enabled by default) because it really is a stylistic question rather than a correctness issue.

I wonder why we don't do this yet...

For exactly the reason that @lrhn gave: we convert the URI to a file path and ask the OS to resolve it. If the OS reports that the file exists, then the file exists.

I think the lint rule here would then report when you import a path which doesn't exist, considering case.

It isn't clear to me whether that approach would work or not. If we're on a case sensitive OS then we'll already get a diagnostic because the target of the URI doesn't exist, and we don't want to double report the issue. If we're on a case insensitive OS, and ask for a file using a path that's "different" from what the OS has for the file (where "different" means different when case is taken into account), which path will the resulting File instance have? Will the OS report the same path we gave it, or will it canonicalize the path in some way?

On the other hand, a lint to enforce only using lower-case letters in URIs won't prevent someone on a case insensitive OS from committing a file with a mixed-case path without realizing it.

srawlins commented 2 years ago

Ah I see. Yes then a lint rule makes sense. Something you can opt-in to, understanding what it means.

lrhn commented 2 years ago

It's probably outside the analyzer's scope to lint that file names are actually lower-case (because how do you even check). Checking import URIs should work.

The Pub tool could perhaps check that you only upload lower-case files. IIRC, it gets a zip file of the package directory, so it can check the file names in the zip without going through a file system. (@jonasfj)

pq commented 2 years ago

The Pub tool could perhaps check that you only upload lower-case files.

Interestingly, file_names should flag these and is part of the core lint set. I'd expect these pub packages are already getting their scores dinged...

lrhn commented 2 years ago

Will the file_names lint also check directory names, or is it only library names? (The documentation only mentions library names, and probably even only library names matching \w+.dart) The problem described in this issue is a directory name.

I'd want a warning if any part of an import path contains an upper-case character (I don't care about - vs _ at all, those work on all platforms), and I'd like a Pub warning if I try to upload any file or directory with an upper-case character in it.

pq commented 2 years ago

Will the file_names lint also check directory names, or is it only library names?

Ah! Sorry, I wasn't looking closely. file_names does not check directories so it's not sufficient. We could definitely consider broadening it to look at the package-relative path. It'd be interesting to scan some pub packages (and Google3) and see how disruptive this change might be.

I'd like a Pub warning if I try to upload any file or directory with an upper-case character in it.

I'd be curious to get @jonasfj's thoughts but sounds good to me.

AbhishekDoshi26 commented 2 years ago

@pq just a question, why just. the packages? Can the lint rules throw warnings even for the local folders? Because if I have a file as screens/home.dart and if I import it as Screens/home.dart it works on macOS. But when such imports are moved into pipelines, it breaks and gets really difficult to debug.

pq commented 2 years ago

just a question, why just. the packages?

Sorry, I wasn't too clear. I just meant that we'd not want to check absolute path (which was probably obvious). Definitely, I think we'd want to flag Screens/home.dart locally. If we pile this onto file_names this would get reported on home.dart btw and not on the local import of it...

jonasfj commented 2 years ago

I'd like a Pub warning if I try to upload any file or directory with an upper-case character in it.

We did a while ago forbid uploads when package contains two files with the same name (case-insensitive), because extracting such packages on MacOS and Windows is problematic.

I think it would be reasonably to warn against upper-case letters in Dart file names when publishing (ie. only for *.dart files). But it can only be a warning, since people might be need to include files for testing reasons.

IMO, we could make hint in the analyzer that discourages imports when the casing doesn't match the casing on the file-system. Afterall Windows and MacOS are typically case-preserving, and if your import-statement doesn't match the casing it'll cause problems on Linux, and generally indicates that you've made a bug.

It would also be preferable to surface this early, rather than only surfacing it packages are publishing. This could still occur within a single repository.

lrhn commented 2 years ago

The problem is in detecting the case of the file in a case-insensitive file system.

Asking if a file exists at another capitalization is not sufficient, the other capitalization could exist on a case-sensitive file system.

You'd probably have to:

Check if an imported file exists.
If so, list the files of the directory, presumably getting the stored case of the files, and check if there is no file with the same capitalization as the import.
Since directories have cases too, repeat that for each directory all the way up to the root, to check that the entire path has the same casing as the import URI.

That's a lot of extra work.

Maybe there is a way to have the system canonicalize an arbitrary path (we'd want that anyway). I don't think it's trivial (https://stackoverflow.com/questions/7195337/how-do-i-get-a-path-with-the-correct-canonical-case-in-powershell), and we'd need to do it on each supported platform, or at least those where we don't know for sure that all file systems are case sensitive. And in a way where canonicalization doesn't also resolve symbolic links. Also, Unicode!

kissa-hh commented 1 year ago

Just adding a case that might highlight the potential hazards of this issue.

We had import "services/services.dart; in our code base, and someone accidentally added import "services/Services.dart";. Suddenly we had two Services classes in our runtime, and all of our services (which were static members in that class) were instantiated twice. It took our team quite a while to realize what was going on and even then we found it hard to believe that a static member could be initialized twice. I can imagine that a bug like that could have serious consequences.

I'd prefer this to be solved at the language level, not as a linter rule. My preferred solutions:

prevent importing "File.dart" if "File.dart" is not the canonical filename (but, as mentioned, this might be hard to implement; the C standard library also doesn't seem to have an easy way to find out the canonical filename)
prevent importing "File.dart" and "file.dart" in the same program
make importing "File.dart" and "file.dart" the same thing (by lowercasing all filenames before registering them)

kissa-hh commented 1 year ago

Here's a test case to show what was going on:

main.dart:

import 'Test.dart' as t1;
import 'test.dart' as t2;

void main() {
    var n = t1.Test.x;
    var n2 = t2.Test.x;
}

test.dart:

import 'Test.dart' as t1;
import 'test.dart' as t2;

void main() {
    var n = t1.Test.x;
    var n2 = t2.Test.x;
}

Result:

f() called!
f() called!

(Expected result: that program would either produce an compile error or just print the message once.)

sorewanya commented 1 year ago

I have similar problem in imports: file structure:

this_page.dart ; this_entity.dart ; this_entity.g.dart; 
that_entity.dart ; that_entity.g.dart ;

this_page.dart:

import 'this_entity.dart';
class ThisPage ...
final File file;
...

that_entity.dart:

import 'package:copy_with_extension/copy_with_extension.dart';

part 'that_entity.g.dart';

@CopyWith()
class ThatEntity ...
...

1)copy this_page.dart >> that_page.dart 2)in "that_page.dart" using "select all" (ctrl+F2 in VSC) i replace "This" >> "That" in classes name, but forget, what it real "select ALL" - imports lines is ALL too.

result of this: Dart don't find copyWith() extension from that_entity.g.dart file. The method 'copyWith' isn't defined for the type 'ThatEntity'.

the ThatEntity itself show "That_entity.dart" in tooltip, all property and methods from this file are available for use, its work: final thatFile = ThatEntity().file;

solution: T=>t in import

I think its not feature, but cant named bug )) (its not copy_with_extension problem! do not work nothing from all .g. .freezed. files)

dart-lang / linter

Dart imports are not case sensitive #3244

Summary of Issue:

Steps to Reproduce