Fix 17699 - Importing a module that has both modulename.d and modulename/package.d should be an error

[dkorpel]

[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
✓	17699	enhancement	Importing a module that has both modulename.d and modulename/package.d should be an error

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#14407"

[Geod24]

Should it, really ? I don't see it as particularly problematic, and the solution is to add one FS access for every module we open, which is IMO pretty bad.

[ibuclaw]

What happens if you import pkg17699.datetime.foo; when the package module is in this ambiguous structure? What happens if the module declarations are missing?

[dkorpel]

Should it, really ? I don't see it as particularly problematic,

See the bugzilla issue for rationale.

and the solution is to add one FS access for every module we open, which is IMO pretty bad.

How much do you think it affects build times?

[dkorpel]

What happens if you import pkg17699.datetime.foo; when the package module is in this ambiguous structure?

The same as before.

What happens if the module declarations are missing?

I don't think they affect anything here.

[Geod24]

See the bugzilla issue for rationale.

Yeah that rationale is shaky. "If one version of Phobos is unzipped over another" isn't a great start, as it suffers from numerous other bugs as soon as the structure is touched.

How much do you think it affects build times?

This instance, probably little, however it's the little things that matter. Any time we have to do an extra check / some extra work in the general case for a corner case, we should ask ourselves if it's the right thing to do. This case being, IMO, a textbook example.

[ibuclaw]

What happens if you import pkg17699.datetime.foo; when the package module is in this ambiguous structure?

The same as before.

What happens if the module declarations are missing?

I don't think they affect anything here.

I recall encountering a segfault when reducing something unrelated a few months back, didn't have time to synthesize. This file structure looked familiar though.

[dkorpel]

This case being, IMO, a textbook example.

Textbook example of what? Do you have suggestions for a different implementation or should the issue just be closed as WONTFIX?

[ibuclaw]

I recall encountering a segfault when reducing something unrelated a few months back, didn't have time to synthesize. This file structure looked familiar though.

https://issues.dlang.org/show_bug.cgi?id=23327

[schveiguy]

Yeah that rationale is shaky. "If one version of Phobos is unzipped over another" isn't a great start, as it suffers from numerous other bugs as soon as the structure is touched.

Rationale written in 2017, when a common mechanism to install your new compiler was to unzip it over the old one.

and the solution is to add one FS access for every module we open

We are already opening multiple for a package.d form, plus opening 5 files per import directory to see if it's there.

Let's say you have a dub project with no dependencies. importing std.stdio stats 12 files before it eventually opens the right one (tested with strace):

stat("std/stdio.di", 0x7ffde5204920)    = -1 ENOENT (No such file or directory)
stat("std/stdio.d", 0x7ffde5204920)     = -1 ENOENT (No such file or directory)
stat("std/stdio.i", 0x7ffde5204920)     = -1 ENOENT (No such file or directory)
stat("std/stdio.c", 0x7ffde5204920)     = -1 ENOENT (No such file or directory)
stat("std/stdio", 0x7ffde5204920)       = -1 ENOENT (No such file or directory)
stat("source/std/stdio.di", 0x7ffde5204920) = -1 ENOENT (No such file or directory)
stat("source/std/stdio.d", 0x7ffde5204920) = -1 ENOENT (No such file or directory)
stat("source/std/stdio.i", 0x7ffde5204920) = -1 ENOENT (No such file or directory)
stat("source/std/stdio.c", 0x7ffde5204920) = -1 ENOENT (No such file or directory)
stat("source/std/stdio", 0x7ffde5204920) = -1 ENOENT (No such file or directory)
stat("/home/steves/.dvm/compilers/dmd-2.099.1/linux/bin/../../src/phobos/std/stdio.di", 0x7ffde5204920) = -1 ENOENT (No such file or directory)
stat("/home/steves/.dvm/compilers/dmd-2.099.1/linux/bin/../../src/phobos/std/stdio.d", {st_mode=S_IFREG|0664, st_size=173318, ...}) = 0

Add 5 more stats for each dependency.

Adding one more stat doesn't seem like it will make a huge difference in the performance.

[RazvanN7]

In this case I agree with @Geod24 . Even if the penalty is small compared to what is already being done, each little corner case check adds to the churn. On the hand, the benefit is almost non existent in my opinion, since the frequency of such a scenario is very small.

[schveiguy]

Even if the penalty is small compared to what is already being done

Please measure. If this is going to add a few ns per import, I think it's worth avoiding causing a 1 hour puzzlement when it does happen.

Note, ImportC was worth adding 2N extra stats (two per import path), and nobody blinked. This is one extra stat total, regardless of where the import is found.

[RazvanN7]

My point was more about adding ugliness to support a narrow use case, rather than performance. I don't think that the compiler should take care of every esoteric use case that arises.

That being said, I really don't care that much about this case. If others decide that this is worth it, I'm not going to block this addition in any way.

[schveiguy]

FYI, #14582 reduces the number of stats by over 90% for my project. After that PR is merged, an extra check is moot.