Skip to content

Conversation

@marler8997
Copy link
Contributor

@marler8997 marler8997 commented Feb 12, 2018

This PR unifies the behavior of imports regardless of what modules were given on the command line (https://issues.dlang.org/show_bug.cgi?id=15086). Consider the following:

// main.d
import foo;
// foo.d
module bar;

The above code will fail if compiled together, i.e.

dmd -c main.d foo.d
Error: module `bar` from file foo.d must be imported with 'import bar;'

However, if you compile them using 2 invocations, it will succeed:

dmd -c main.d
dmd -c foo.d

This PR unifies both cases by making them both fail with the same error message.

@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 12, 2018

Thanks for your pull request and interest in making D better, @marler8997! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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
15086 critical import doesn't verify module declaration

@marler8997 marler8997 force-pushed the verifyModuleName branch 2 times, most recently from af2f88d to 2cc2b19 Compare February 12, 2018 22:11
@marler8997 marler8997 changed the title Deprecated full import/module name mismatch Fix issue 15086: import doesn't verify module declaration Feb 12, 2018
@marler8997 marler8997 force-pushed the verifyModuleName branch 5 times, most recently from e0574ff to 52f26e9 Compare February 12, 2018 22:24
@aG0aep6G
Copy link
Contributor

Note that this will not be an error if the import name matches the module name at least partially, i.e.

// main.d
import foo.bar1
import foo.bar2;

// foo/bar1.d
module pkg.foo.bar1; // OK (foo.bar1 partially matches pkg.foo.bar1)

// foo/bar2.d
module bar2; // OK (foo.bar2 partially matches bar2)

I don't see how this makes sense. The modules are pkg.foo.bar1 and bar2, and they must be imported as such.

Fixing just the test case of 15086 is fine by me, though. As long as this doesn't allow more broken stuff than we currently have. I can just file another issue for the partial matching thing.

@marler8997
Copy link
Contributor Author

marler8997 commented Feb 12, 2018

I don't see how this makes sense. The modules are pkg.foo.bar1 and bar2, and they must be imported as such.

This is currently being debated between @WalterBright, @andralex and myself (see #7778). They currently don't see this as a problem.

Fixing just the test case of 15086 is fine by me, though. As long as this doesn't allow more broken stuff than we currently have. I can just file another issue for the partial matching thing.

This PR does not allow anything that wasn't already allowed, it is only restricting the use case described.

@marler8997
Copy link
Contributor Author

marler8997 commented Feb 13, 2018

I should mention that importing a module using it's filename, when it's module name does not match is already an error, it just doesn't get detected if you don't pass both files to the compiler on the same invocation. i.e.

// main.d
import foo;
void main() { }
// foo.d
module bar;

If you compile them separately using 2 invocations

dmd -c main.d
dmd -c foo.d

It works, but if you compile them with one invocation (note still using -c to compile seperately) then it will fail:

dmd -c main.d foo.d

it will fail with Error: module bar from file foo.d must be imported with 'import bar;'.

This should not be. Either both cases should fail or both should pass.

Note that by continuing to support this feature, an import statement can result in different behavior depending on how the compiler is invoked. To demonstrate, consider

import foo;

What does this mean? Currently, it does not necessarily mean you are importing a module named foo. When the import is processed, if there so happens to be a module named foo that was loaded, then it will use that module, however, if no module named foo has been loaded yet, then it will search the file system for a file named foo, which could contain a module with any name. This means that the imported module can change depending on the other modules that were passed on the command line. This PR aims to decouple how a module is compiled with the set of other modules passed to the compiler by requiring that the import name at least partially match the module name it ends up importing.

@marler8997 marler8997 changed the title Fix issue 15086: import doesn't verify module declaration Interpret imports the same regardless of other modules passed on the command line Feb 13, 2018
@andralex
Copy link
Member

Had a long discussion with @WalterBright. Of the cases initially presented, this is the one with most merit. The following will focus on it, although certain considerations apply beyond.

Meta: the incumbent behavior has extra weight by means of being incumbent. We need to make a strong case to displace it.

History: initially module declarations were a lot stricter. That turned out early on to be too rigid to be workable. That doesn't dismiss revisiting it outright, but does stand as a warning.

The case for this PR rests on the notion there should be some invariance of build results with changing build conditions. E.g. "Either both cases should fail or both should pass" (and let me add: "... and create the same binary"). That actually doesn't seem to be a strong argument: any number of changes in the way binaries are built influence the result, and some are not workable. Furthermore, the added value of changing that should be argued (see "Meta"): what problem is this fixing? The error message plainly tells what must be done. We should enhance it.

A stronger case would be made by showing how unwitting code compiles in two distinct and plausible cases, yet behaves differently at runtime. "Unwitting" is an important detail because matters can be manipulated on the filesystem side - see below.

The second point made is that simple imports such as import foo; leave detail out. First off, a brief detour - in languages like C and C++, the directive #include "foo.h" is similarly vague. Both can mean many things by means of file and directory links, mounted drives, uses of -I, etc etc.

What import foo; does mean is that once the import succeeds, the importing module has access to the artifacts imported under the name foo. Intuitively it makes a lot of sense that the importing module has some flexibility in choosing the imported module name, otherwise import xyz = foo; would be fraught with trouble.

Another good argument in favor of this PR could be constructed by showing that semantics of a successful build depend on the order in which files are specified. Order dependency is easy to characterize as undesirable.

@marler8997
Copy link
Contributor Author

marler8997 commented Feb 13, 2018

"Either both cases should fail or both should pass"...That actually doesn't seem to be a strong argument

This is what I believe to be the strongest argument for this PR. If this argument does not justify it then it's dead. Closing.

@marler8997 marler8997 closed this Feb 13, 2018
@marler8997
Copy link
Contributor Author

In the future, I think we can be more efficient with our time if concerns about a bug not being a bug are voiced before a PR is created for it. Especially when that bug has been around for 2 and a half years.

@andralex
Copy link
Member

@marler8997 You are certainly right, and I apologize. This has been off my radar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants