Fix Issue 17181 - Local imports in templates should be added to impor… #9221

RazvanN7 · 2019-01-08T15:27:38Z

…ted modules list of module that instantiated it

I don't know how to test this. @andralex maybe check your PR against this patch?

dlang-bot · 2019-01-08T15:27:40Z

Thanks for your pull request and interest in making D better, @RazvanN7! 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
✓	17181	normal	Local imports in templates should be added to imported modules list of module that instantiated it

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + dmd#9221"

thewilsonator

Tests?

WalterBright · 2019-01-09T08:56:50Z

src/dmd/dsymbolsem.d

-            sc._module.aimports.push(imp.mod);
+
+            // if inside a template instantiation, the instantianting
+            // module gets the import.


auto m = (sc.minst && sc.tinst) ? sc.minst : sc._module; m.aimports.push(imp.mod);

4 lines of code to 2.

Might want to add a link to 17181 for rationale.

RazvanN7 · 2019-01-09T15:17:18Z

@WalterBright done.
@thewilsonator Added test.

RazvanN7 · 2019-01-09T15:25:16Z

test/runnable/test17181.d

+    return imports.test17181a.abc(1);
+}
+
+static this() { assert(a == 2); }


A few explanations about this test:

-> before this patch, for the code in the test the compiler issued a wrong cyclic dependency error. This happened because the instantiation of abc, although was in module test17181, was regarded as being in test17181a. This made it look like test17181 relies on test17181a which relies on test17181b, which in turn relies again on test17181a. But that is not true, since the import of test17181b is in a template, therefore the import should be in test17181.

-> the correct order for the static constructors should be: test17181a->test17181b->test17181.
-> the test makes sure that when the static constructor for test117181 is run, the last static constructor ran was the one from test17181b.

rainers · 2019-01-09T21:27:34Z

I suspect there are more complications: if a template is instantiated by mutliple modules, the import is added to the module that happens to be analyzed first. Other modules that reuse the same template instance need to have a dependency on the import, too.

…ted modules list of module that instantiated it

RazvanN7 · 2019-01-10T11:38:36Z

I suspect there are more complications: if a template is instantiated by mutliple modules, the import is added to the module that happens to be analyzed first. Other modules that reuse the same template instance need to have a dependency on the import, too.

I added a fix, but I have no idea how to test this case. I don't even think it's possible.
The solution is to store the modules that a template instance imports. Whenever an instance is reused, the modules are added to the instantiating module import list.

wilzbach · 2019-01-10T11:40:27Z

I suspect there are more complications: if a template is instantiated by mutliple modules, the import is added to the module that happens to be analyzed first. Other modules that reuse the same template instance need to have a dependency on the import, too.

I added a fix, but I have no idea how to test this case. I don't even think it's possible.

Custom shellscript with multiple EXTRA_FILES?
In the worst case you could always grep on the created (and dumped) object file.
There are a few shell tests which already do similar things.

RazvanN7 · 2019-01-10T11:41:32Z

Custom shellscript with multiple EXTRA_FILES?
In the worst case you could always grep on the created (and dumped) object file.
There are a few shell tests which already do similar things.

You can't see imports in object files. We are talking aboud seeing an internal list in the compiler.

jacob-carlborg · 2019-01-10T11:48:17Z

We are talking aboud seeing an internal list in the compiler.

Use the DMD frontend as a library. But you need to install Dub for that. I tried to that here [1], but it turned out Dub wasn't available on the auto-tester machines.

[1] https://github.com/dlang/dmd/pull/8528/files

wilzbach · 2019-01-10T12:39:58Z

You can't see imports in object files. We are talking aboud seeing an internal list in the compiler.

Maybe grep on dmd -v then?

But you need to install Dub for that.

Though the existing dub tests for the DMD frontend as a library are already tested on CircleCi.

jacob-carlborg · 2019-01-10T13:40:06Z

Though the existing dub tests for the DMD frontend as a library are already tested on CircleCi.

But not on all platforms.

RazvanN7 · 2019-01-10T14:11:52Z

I think that we can merge this as is. We have a test for the original problem and its observable effect. The problem that @rainers specified, even though it is solved by this PR it didn't have any observable effects.

andralex

LGTM. @WalterBright? @wilzbach? Any others?

rainers

LGTM. A test case that shows the dependency issue with reusing the template instance should be feasible with seperate compilation (especially through library) but probably not worth the trouble.

An interesting change is that a module that only contains templates with imports, but no global imports, now no longer needs to be compiled to an object file as it should never appear in a dependency chain itself. Maybe worth a test case...

rainers · 2019-01-10T23:00:46Z

You can't see imports in object files.

You can if you dump the data segments, too. You'll find the imports that are in the dependency chain in the __ModuleInfo section. See the ASM of https://run.dlang.io/is/6rUcxc

PetarKirov · 2019-01-13T10:01:05Z

@RazvanN7 are you up to adding a test, as per @rainers's comment?

wilzbach · 2019-01-13T10:29:02Z

@RazvanN7 are you up to adding a test, as per @rainers's comment?

Yes please.

WE SHOULD REALLY REALLY REALLY LEARN FROM PAST MISTAKES.

[stable] Follow-up on #9221 merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>

dlang-bot added the Severity:Bug Fix label Jan 8, 2019

RazvanN7 mentioned this pull request Jan 8, 2019

Add imported for inline imports dlang/druntime#1756

Merged

wilzbach added the Review:Needs Tests label Jan 8, 2019

thewilsonator reviewed Jan 8, 2019

View reviewed changes

WalterBright reviewed Jan 9, 2019

View reviewed changes

RazvanN7 force-pushed the Issue_17181 branch from 2564e4d to 303db3d Compare January 9, 2019 15:16

RazvanN7 removed the Review:Needs Tests label Jan 9, 2019

RazvanN7 commented Jan 9, 2019

View reviewed changes

Fix Issue 17181 - Local imports in templates should be added to impor…

db784a8

…ted modules list of module that instantiated it

RazvanN7 force-pushed the Issue_17181 branch from 303db3d to db784a8 Compare January 10, 2019 11:35

andralex approved these changes Jan 10, 2019

View reviewed changes

rainers approved these changes Jan 10, 2019

View reviewed changes

andralex merged commit 86b608a into dlang:master Jan 11, 2019

kinke added a commit to kinke/dmd that referenced this pull request Mar 4, 2020

Follow-up on dlang#9221

b1e245d

dlang-bot added a commit that referenced this pull request Mar 10, 2020

Merge pull request #10864 from kinke/needmoduleinfo

345fac2

[stable] Follow-up on #9221 merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>

MartinNowak mentioned this pull request Apr 12, 2020

merge stable #11027

Merged

Uh oh!

Fix Issue 17181 - Local imports in templates should be added to impor… #9221

Fix Issue 17181 - Local imports in templates should be added to impor… #9221

Uh oh!

Conversation

RazvanN7 commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jan 8, 2019

Bugzilla references

Testing this PR locally

Uh oh!

thewilsonator left a comment

Choose a reason for hiding this comment

Uh oh!

WalterBright Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

WalterBright Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Jan 9, 2019

Uh oh!

RazvanN7 Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainers commented Jan 9, 2019

Uh oh!

RazvanN7 commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Jan 10, 2019

Uh oh!

RazvanN7 commented Jan 10, 2019

Uh oh!

jacob-carlborg commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Jan 10, 2019

Uh oh!

jacob-carlborg commented Jan 10, 2019

Uh oh!

RazvanN7 commented Jan 10, 2019

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

rainers left a comment

Choose a reason for hiding this comment

Uh oh!

rainers commented Jan 10, 2019

Uh oh!

PetarKirov commented Jan 13, 2019

Uh oh!

wilzbach commented Jan 13, 2019

WE SHOULD REALLY REALLY REALLY LEARN FROM PAST MISTAKES.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

RazvanN7 commented Jan 8, 2019 •

edited

Loading

RazvanN7 Jan 9, 2019 •

edited

Loading

RazvanN7 commented Jan 10, 2019 •

edited

Loading

jacob-carlborg commented Jan 10, 2019 •

edited

Loading