Fix 16701 - Remove Restriction of "package.d" Source File Module Forc…

[dkorpel]

…ed to All Lowercase

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	16701	enhancement	Remove Restriction of "package.d" Source File Module Forced to All Lowercase

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14260"

[adamdruppe]

So this would fix that one issue, but the larger issue with package.d is that the compiler forces a filename at all. It should work like the rest of the language, where the filename can match the module name, or it can be told to it.

This change would actually make it even more inconsistent with the rest of the language, by still requiring a filename, but, for once, not being case sensitive.

(that said i do see the practical benefit of being case insensitive if you wanna force the name, but i still maintain that oyu shouldn't be forcing the name at all)

[dkorpel]

It should work like the rest of the language, where the filename can match the module name, or it can be told to it.
(...) you shouldn't be forcing the name at all

I don't follow, what names are being forced to what exactly? A package module must be called "package.d" because it is special, I wouldn't want it to be called something else.

[adamdruppe]

There is zero justification for it to be special (and in fact, it goes directly counter to the stated justification for the feature in the first place, to maintain compatibility with old code!). THAT is the inconsistency with the rest of the language, and the root cause of most these bugs.

[dkorpel]

There is zero justification for it to be special (and in fact, it goes directly counter to the stated justification for the feature in the first place, to maintain compatibility with old code!).

What is the compiler supposed to do with import some_folder; then, if not locate a "package.d" inside it?

[adamdruppe]

The same thing it always does: check some_folder.di an some_folder.d for a module some_folder; declaration.

You can still put module some_folder.foo; in some_folder\foo.d since some_folder/ and some_folder.d are not clashing to begin with.

Of course, since we already have this brain-damaged design, it'd probably have to continue checking some_folder/package.d to retain compatibility, but it can at least start doing the consistent checks in addition to it moving forward and start reducing the other (buggy) special cases.

[ryuukk]

I personally think they should stay lowercase to be consisted with everything else, this change adds unnecessary inconsistencies

[dkorpel]

to be consisted with everything else

What "everything else"? The only other special file I know of is object.d, which is also checked with FileName.equals. File extensions are also checked with FileName.equals. "package.d" being checked with strcmp is the inconsistency.

[ryuukk]

to be consisted with everything else

What "everything else"? The only other special file I know of is object.d, which is also checked with FileName.equals. File extensions are also checked with FileName.equals. "package.d" being checked with strcmp is the inconsistency.

with every other package.d, everything in the std and druntime, filename seems to follow the lower, snake case, even in libraries around, everything is lower case, also it makes it harder to search

[ryuukk]

If that change goes in, we'll need to also update DCD to handle this

https://github.com/dlang-community/DCD/blob/2bfd3d004ff7eda2428353d2ab44f389dfd5b8e0/src/dcd/server/autocomplete/complete.d#L476-L485

[dkorpel]

with every other package.d, everything in the std and druntime, filename seems to follow the lower, snake case, even in libraries around, everything is lower case

This PR doesn't change those files, nor does it change the style convention.

[dkorpel]

If that change goes in, we'll need to also update DCD to handle this

https://github.com/dlang-community/DCD/blob/2bfd3d004ff7eda2428353d2ab44f389dfd5b8e0/src/dcd/server/autocomplete/complete.d#L476-L485

I don't see any case-sensitive filename checks in the linked code

[ryuukk]

with every other package.d, everything in the std and druntime, filename seems to follow the lower, snake case, even in libraries around, everything is lower case

This PR doesn't change those files, nor does it change the style convention.

It doesn't change that but it introduce that possibility now

[ryuukk]

If that change goes in, we'll need to also update DCD to handle this
https://github.com/dlang-community/DCD/blob/2bfd3d004ff7eda2428353d2ab44f389dfd5b8e0/src/dcd/server/autocomplete/complete.d#L476-L485

I don't see any case-sensitive filename checks in the linked code

it builds the path with package.d|di that are expected to be lower case, so it'll fail later if user/library has a Package.d in his folder

[dkorpel]

It doesn't change that but it introduce that possibility now

The compiler isn't there to enforce a style guide. It would be especially weird if it enforces a casing style on "package.d" and nothing else.

[dkorpel]

it builds the path with package.d|di that are expected to be lower case

I don't see code that expects it to be lower case. exists() is not case-sensitive on Windows.

[adamdruppe]

It would be especially weird if it enforces a casing style on "package.d" and nothing else.

When it is looking up modules normally, it assumes the file name matches the module name (unless you give it a list of files, in which case it ignores the name and just checks the module definition - which it doesn't allow for package.d lol).

But when it uses the module name to guess the filename, that is case sensitive because module names are case sensitive in D. The underlying filesystem might return a case-insensitive answer though, which can cause trouble, so the style guide recommends always using all lowercase module names. But on linux, for example, the lookup is case sensitive in D and in the filesystem.

[ryuukk]

It doesn't change that but it introduce that possibility now

The compiler isn't there to enforce a style guide. It would be especially weird if it enforces a casing style on "package.d" and nothing else.

What about other filesystems?

It introduce unnecessary complexity, rules sometimes are beneficial, if some other people in their tools expect it to be lower case no matter what, they'd have to update their tools too, for what benefits? i don't see any, one improvement would be to get rid of the need of package.d as adam suggested

[dkorpel]

What about other filesystems?

This only affects Windows, which has a case insensitive file system. On Linux it's still case sensitive. I don't know what 'other' filesystems you're talking about.

It introduce unnecessary complexity

I'm not a fan of the fact that Windows has a case-insensitive file system and its associated complexity, but DMD releases on Windows and adheres to its choice in most cases. The only exceptions I know of being this PR and #14261 . You could have a stance of enforcing casing even on Windows, but why only "package.d" and not everything else?

one improvement would be to get rid of the need of package.d as adam suggested

I agree, but that's for a later PR. This is a trivial fix.

[Geod24]

So this would fix that one issue, but the larger issue with package.d is that the compiler forces a filename at all. It should work like the rest of the language, where the filename can match the module name, or it can be told to it.

Actually the filename really should the module name. Anything else breaks too easily.
Regarding package.d[i] itself, it would be consistent with the way we treat extensions. I don't really like removing this restriction personally, as I don't think it does any harm, but I don't have a strong opinion one way or the other.

@dkorpel : I don't see any test, and I don't see any change to the lookup code though, so I'm not sure it "fully" work ?

EDIT: Right, I missed the part where this "only works on Windows", also consistent with extensions I suppose.

[RazvanN7]

@dkorpel can you add a test for this?

[dkorpel]

Done. auto-tester failure looks unrelated:

core.exception.AssertError@std/experimental/logger/core.d(1908): unittest failure