Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@import is too lenient with paths on windows #9786

Open
cark opened this issue Sep 16, 2021 · 10 comments
Open

@import is too lenient with paths on windows #9786

cark opened this issue Sep 16, 2021 · 10 comments
Labels
contributor friendly This issue is limited in scope and/or knowledge of Zig internals. enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Milestone

Comments

@cark
Copy link

cark commented Sep 16, 2021

When specifying @import paths with upper/lowercase errors, the build breaks in a non-obvious way.

Here is a minimal example (full source in this repository : https://github.com/cark/zig_windows_issue)

File my_type.zig (all lowercase filename):

pub const MyType = struct {
    value: usize,
    pub fn sum(self: MyType, my_type: MyType) MyType {
        return MyType{.value = my_type.value + self.value};
    }
};

File my_user.zig (we're importing my_type):

const std = @import("std");
const expect = std.testing.expect;

const MyType1 = @import("my_type.zig").MyType;
const MyType2 = @import("my_type.zig").MyType;
const MyType3 = @import("my_type.Zig").MyType; // capital Z 

test "sum1" { // this works fine
    const  mt1 = MyType1{.value = 12};
    try expect(mt1.value == 12);

    const mt2 = MyType2{.value  = 13};
    try expect(mt2.value == 13);

    const result = mt1.sum(mt2);
    try expect(result.value == 25);
}

test "sum2" { // this works fine
    const  mt31 = MyType3{.value = 12};
    try expect(mt31.value == 12);

    const mt32 = MyType3{.value  = 13};
    try expect(mt32.value == 13);

    const result = mt31.sum(mt32);
    try expect(result.value == 25);
}

test "sum3" {  // This errors out
    const  mt1 = MyType1{.value = 12};
    try expect(mt1.value == 12);

    const mt3 = MyType3{.value  = 13};
    try expect(mt3.value == 13);

// .\src\my_user.zig:*:*: error: expected type 'src.my_type.MyType', found 'src.my_type.MyType'
    const result = mt1.sum(mt3);
    try expect(result.value == 25);
}

It looks to me like windows is being helpful in accepting the capital Z in my_type.Zig, but the compiler identifies the types according to the filename specified in the @import.

I'm guessing the same compilation would be instantly rejected on unix systems.

You will notice that the error message is particularly unhelpful. In a larger code base, it might be a pain to pinpoint the issue.

@marler8997
Copy link
Contributor

marler8997 commented Sep 16, 2021

Yes zig is vulnerable to this type of error because of the inconsistent handling of case sensitivity between Windows filesystems and nix ones. As far as I know, Windows is the only OS that uses case insensitive file systems. I'd propose that whenever zig performs an import on Windows, it verifies that the casing used to import matches the actual casing of the filename as stored on the filesystem (not just that the file exists or can be opened, because that is case insensitive). Without this, you could have projects that compile on Windows but don't compile on linux, i.e.

foo.zig:

main.zig:

const foo = @import("Foo.zig");

This would compile on Windows but not on linux.

@SpexGuy
Copy link
Contributor

SpexGuy commented Sep 16, 2021

I agree, though it will be important that the error message reflects this properly. Windows users may expect the file lookup to be case insensitive, and be confused by "cannot find Foo.zig" when foo.zig exists. We'll need a special case error note on windows to mention that the casing is wrong.

@AssortedFantasy
Copy link
Sponsor

How does this work the other way around? Would a project which uses multiple files which only differ in case get screwed in windows but work on linux?

Granted, making several several source files which only differ in case sounds like a terrible idea anyhow, and I doubr anyone would be doing that.

@mikdusan
Copy link
Member

mikdusan commented Sep 17, 2021

As far as I know, Windows is the only OS that uses case insensitive file systems.

[fyi]

macos APFS (and HFS+) by default are case-insensitive. Files can equally be opened in mixed case. @import("foo.Zig") would also succeed in importing "foo.zig". It is/was known that apps on macos do not test for case-sensitive file systems, eg. there were reports that Adobe Photoshop had serious issues because binaries hardcoded a case mis-match to some installed file names.

and if anyone cares, Apple's other operating systems (iOS, iPadOS, etc) actually use case-sensitive APFS.

@marler8997
Copy link
Contributor

How does this work the other way around? Would a project which uses multiple files which only differ in case get screwed in windows but work on linux?

Yes actually. A while back I tried cloning the linux kernel repository on windows and it had at least 1 file that only differed in casing and git wasn't able to clone it. It's actually good that git was able to detect this issue instead of just overwriting the file with the contents of one of the actual files.


macos APFS (and HFS+) by default are case-insensitive.

@mikdusan thanks for the info

@andrewrk andrewrk added this to the 0.9.0 milestone Sep 17, 2021
@andrewrk andrewrk added contributor friendly This issue is limited in scope and/or knowledge of Zig internals. enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness. labels Sep 17, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 Nov 22, 2021
@fivemoreminix
Copy link

Windows users may expect the file lookup to be case insensitive, and be confused by "cannot find Foo.zig" when foo.zig exists. We'll need a special case error note on windows to mention that the casing is wrong.

macos APFS (and HFS+) by default are case-insensitive.

I think this would be a good reason to have an unmatched casing error present on all operating systems. Just good QOL

@michalsieron
Copy link

FYI you can have case folding enabled on ext4
https://www.collabora.com/news-and-blog/blog/2020/08/27/using-the-linux-kernel-case-insensitive-feature-in-ext4/

so @fivemoreminix's proposal to just error on imports with wrong casing seems to be the solution. it would also lead to fewer permutations of source code #7399 (comment)

@jorangreef
Copy link
Sponsor Contributor

jorangreef commented May 16, 2022

If it helps, I spent some time investigating these quirks for a file synchronization program a few years ago.

IIRC, Windows also has another quirk where leading spaces are truncated, so "my_type.zig" and " my_type.zig" will both match.

A similar kind of problem can also manifest on some macOS filesystems, which have the equivalent of Windows' case-insensitivity but for Unicode normal forms. For example, here Windows will do the right thing and be precise with a UTF-8 filename in NFC form, but then macOS will go and match this NFC form also with its own custom NFD form, so you can have multiple filenames all mapping to the same NFD form. macOS will also do normalization and lose the original NFC form when the file is written, so there's no way to go back. The mapping table for this is proprietary and slightly different to how most Unicode libraries do NFD-to-NFC-to-NFD conversions so you can't always roundtrip it.

It's important to know that these quirks are not actually OS-specific, it's a little worse than that—because they're filesystem-specific. For example, macOS can work with volumes that are form-insensitive or form-sensitive. Same I believe for Windows with respect to case. It all depends on the mount point.

There is hope. The best way to tackle these is always to treat filenames as data, preserve filenames, preserve case, and preserve form. Never normalize when storing data. Keep data pristine at the highest resolution possible. You can downsample only for comparison, but not for storing, because once resolution is lost, you can never upsample.

Here's the full guide I wrote about this, once upon a time for Node.js: https://nodejs.org/en/docs/guides/working-with-different-filesystems/

@arBmind
Copy link
Contributor

arBmind commented Jul 25, 2022

Looking back to this issue, we still have to decide what we want here.

a) Working on non case sensitive file systems and importing the wrong casing leads to issues for most other developers. So we want to report this as an error.
b) Checking the path name from the file system against the value given by the user may not work on all file systems. Like having da DOS FAT.

My suggestion would be to enable this check by default and have a compiler option to disable it, in the rare cases, where it does not work properly. If users report issues for certain file systems we might disable the check by default if we detect it.

@andrewrk andrewrk modified the milestones: 0.10.0, 0.10.1 Oct 12, 2022
@Vexu Vexu modified the milestones: 0.10.1, 0.11.0 Dec 5, 2022
@andrewrk andrewrk modified the milestones: 0.11.0, 0.12.0 Apr 9, 2023
@EspeuteClement
Copy link
Sponsor Contributor

I can confirm that this bug is still present in 0.12.0-dev.1327+256ab68a9

file foo.zig :

const Foo = @This();

a: u8 = 1,
b: u8 = 2,

pub fn add(self: Foo) u16 {
    return self.a + self.b;
}

file main.zig :

const std = @import("std");
const Foo = @import("foo.zig");
const FOO = @import("FOO.zig");

test {
    var foo = Foo{};
    std.debug.print("{d}", .{FOO.add(foo)});
}

zig test main.zig :

main.zig:7:38: error: expected type 'FOO', found 'foo'
    std.debug.print("{d}", .{FOO.add(foo)});
                                     ^~~
foo.zig:1:1: note: struct declared here
const Foo = @This();
^~~~~
FOO.zig:1:1: note: struct declared here
const Foo = @This();
^~~~~
FOO.zig:6:18: note: parameter type declared here
pub fn add(self: Foo) u16 {

changing the FOO import to @import("foo.zig") fixes the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor friendly This issue is limited in scope and/or knowledge of Zig internals. enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Projects
None yet
Development

Successfully merging a pull request may close this issue.