Comment #0 by peter.alexander.au — 2013-06-16T06:51:39Z
Imports local to a function are able to hide local symbols, which can lead to subtle breakage when libraries change and unexpected behaviour:
void main()
{
import std.stdio;
string message = "Hello, world!";
writeln(message);
}
Here, if a symbol "message" was added to std.stdio then the function would use that message instead of the local message. This could cause subtle breakage when users update to libraries that have added new symbols.
Local imports should behave the same as module-level imports, except the symbols are only visible in the local scope. This prevents any sort of unexpected behaviour without hindering the power of local imports. Ambiguous names can always be disambiguated using full symbol qualification.
Comment #1 by mk — 2013-06-16T10:45:23Z
*** This issue has been marked as a duplicate of issue 7329 ***
Comment #2 by peter.alexander.au — 2013-06-16T11:44:54Z
Example was invalid, here's a better one:
string message = "Hello, world!";
void main()
{
import std.stdio;
writeln(message);
}
Also, re-opening. This isn't a dupe. The other bug is just that the semantics are undocumented, not that the behaviour is incorrect. If the current behaviour is documented then the other bug may be closed, but this bug will still be present, so they are not the same bug.
Comment #3 by dbugreporter — 2014-08-28T07:12:18Z
The fact that local imports hide things instead of properly detecting conflicts is a dangerous bug. I actually almost trashed a bunch of files because of it since I had something like this:
import std.stdio;
void main() {
import std.file; // just wanted to use dirEntries!
auto files = dirEntries("importantfiles", "*.{png}", SpanMode.depth);
foreach (f; files) {
auto hdr = read_header(f.name);
writeln(f.name, ":");
writefln("%d x %d", hdr.width, hdr.height);
}
}
Luckily, I had that read_header call *after* the first writeln, and it of course threw and I only lost one file (and it wasn't actually important). I know (now!) that the docs say this about local (or "scoped") imports: "Imported symbols may hide symbols from outer scopes", but this is damn insane. It's inconsistent and surprising.
Comment #4 by dbugreporter — 2014-08-28T07:45:18Z
Sorry, that writeln(f.name, ":"); was actually writeln(f.name, ":"); as in, it called std.file.write instead of std.stdio.write as I intended.
Comment #5 by dbugreporter — 2014-08-28T07:46:19Z
I meant write(f.name, ":"); I can't type shit today...
Comment #6 by andrei — 2014-09-23T18:34:17Z
An example that doesn't compile today for the sake of illustration:
void main(string[] group)
{
import std.algorithm, std.stdio;
writeln(group);
}
This is a critical bug. I'm raising it accordingly.
One simple idea (that would nevertheless break existing code): only allow "static import xyz;" and "import xyz : sym1, sym2;" at local scope.
Comment #7 by hsteoh — 2014-09-23T18:41:31Z
Most blatant illustration of this bug:
------
import std.stdio;
void func(string text) {
import std.conv;
writeln(text);
}
void main() {
func("Hello world");
}
------
Program output:
------
------
:-)
Comment #8 by bearophile_hugs — 2014-09-23T20:25:00Z
Proposal:
Free symbol are resolved from the inner scope to the outer scope WITHOUT considering imports. If that lookup fails, the symbol is searched in import from the inner scope to the outer scope.
A variant is to aggregate import into the inner scope.
This solution has various advantages:
- import cannot hijack in module symbols implicitly.
- import do not need to be processed if local lookup succeed, and doing less work is always a way to make the compiler faster.
The main difference between the first proposal and the variant is that that in the first proposal, an inner imported module's symbol can hijack an outer imported one. In the variant, it doesn't happen, but the import lookup phase will be much heavier, as the compiler must look into all imported module at once.
Comment #10 by hsteoh — 2014-10-07T19:39:36Z
@deadalnix: Unfortunately this would require some rather big changes in dmdfe... currently, imports are implemented by actually inserting symbols from the imported module into the symbol table of the current scope. So either we would have to hack the symbol table to mark imported symbols as being imported, and modify symbol lookup to ignore such symbols the first time round; or, we'd have to introduce an additional symbol table per lexical scope for holding imported symbols, alongside the "normal" symbol table.
Comment #11 by deadalnix — 2014-10-08T00:00:45Z
(In reply to hsteoh from comment #10)
> @deadalnix: Unfortunately this would require some rather big changes in
> dmdfe... currently, imports are implemented by actually inserting symbols
> from the imported module into the symbol table of the current scope. So
> either we would have to hack the symbol table to mark imported symbols as
> being imported, and modify symbol lookup to ignore such symbols the first
> time round; or, we'd have to introduce an additional symbol table per
> lexical scope for holding imported symbols, alongside the "normal" symbol
> table.
We have to sell it to Walter as a way to make D compile faster (as it allow for more lazy imports).
Comment #12 by timon.gehr — 2015-08-19T22:38:19Z
(In reply to deadalnix from comment #9)
> Proposal:
>
> Free symbol are resolved from the inner scope to the outer scope WITHOUT
> considering imports. If that lookup fails, the symbol is searched in import
> from the inner scope to the outer scope.
>
> A variant is to aggregate import into the inner scope.
>
> This solution has various advantages:
> - import cannot hijack in module symbols implicitly.
> - import do not need to be processed if local lookup succeed, and doing
> less work is always a way to make the compiler faster.
>
> The main difference between the first proposal and the variant is that that
> in the first proposal, an inner imported module's symbol can hijack an outer
> imported one. In the variant, it doesn't happen, but the import lookup phase
> will be much heavier, as the compiler must look into all imported module at
> once.
Another solution would be to not have an a priori precedence order for symbols that are imported and symbols that are found in an enclosing scope, and to then do the usual overload resolution. (Think of this like treating enclosing scopes as imports.) This is more in line with the anti-hijacking design philosophy of the module system.
Comment #13 by dfj1esp02 — 2015-08-20T07:44:32Z
Currently documented to work this way.
Comment #14 by k.hara.pg — 2015-08-21T09:38:29Z
We can resolve the issue by applying some rewriting rule.
void test()
{
import a, b;
import c;
stmt1;
improt d;
stmt2;
}
To:
void test()
{
struct __ImportScope1 { // internal namespace
import a, b;
import c;
}
with (__ImportScope1) {
stmt1;
struct __ImportScope2 { // internal namespace
improt d;
}
with (__ImportScope2) {
stmt2;
}
}
}
After that, we can reuse the symbol shadowing check mechanism on WithStatement.
Why can't locally imported symbols work exactly as globally imported symbols, but only accessible in the given scope? That is:
import std.stdio;
// import std.conv; // line 2
void func(string text) {
import std.conv; // after this, it's like line 2 was included
writeln(text); // because std.conv is imported "globally" it doesn't
// mask local var.
}
void main() {
// line 2 isn't included inside here, because it wasn't imported.
func("Hello world");
}
To me, the only benefit of using locally imported symbols is to make the imports implementation details for that function. Having the module just "import globally" at the right time follows the principle of least surprise (they work like all other imports).
There's another travesty this would fix. If you import another module that overloads a function in your current module, you have to *reimport the current module*. i.e.:
a.d:
module a;
void foo(int a) {}
b.d:
module b;
void foo(string s) {}
void main()
{
import a;
import b; // MUST include this
foo("hi"); // for this to compile
foo(0);
}
I admit not to knowing how easy/possible this is to do with the current front end.
Comment #17 by ketmar — 2016-01-19T21:53:04Z
this will pollute module scope with imported symbols.
imagine that i have `mymodule.foo (float v)` function, and defined local `foo (int v)` function. with your "hidden global import" whenever i'll import "mymodule", it will add unexpected `foo` overload for the whole code.
also, it breaks "module importing orger doesn't matter" rule.
Comment #18 by timon.gehr — 2016-01-19T22:30:55Z
(In reply to Ketmar Dark from comment #17)
> this will pollute module scope with imported symbols.
>
> imagine that i have `mymodule.foo (float v)` function, and defined local
> `foo (int v)` function. with your "hidden global import" whenever i'll
> import "mymodule", it will add unexpected `foo` overload for the whole code.
>
> also, it breaks "module importing orger doesn't matter" rule.
Those statements are all incorrect, so maybe you misunderstood the suggestion.
The suggestion is that name lookup should behave as if the set of global imports considered depended on the local scope. The net effect is that imports never hide each other, but symbols of the current module always hide imported symbols.
Comment #19 by ketmar — 2016-01-19T22:37:19Z
ah, i see, thank you. i really misread the suggesion. still, i like Kenji's PR4915 more. ;-) i'd better see "shadowing error", so i can simply rename symbols, and don't guess if there is conflict or not.
Comment #20 by schveiguy — 2016-01-20T20:29:12Z
(In reply to Ketmar Dark from comment #19)
> ah, i see, thank you. i really misread the suggesion. still, i like Kenji's
> PR4915 more. ;-) i'd better see "shadowing error", so i can simply rename
> symbols, and don't guess if there is conflict or not.
Yes, thank you Timon for explaining better.
The main reason I see local imports being useful is this:
import somemodule;
void foo()
{
onlyPlaceSomeModuleIsUsed()
}
Now, one can simply say "wait, somemodule isn't something everyone in the file needs, just foo. I'll move it inside"
And then it breaks. I'd rather just see it work the same. All you are doing is saying who has access to somemodule's symbols, and avoid polluting the module namespace.
Comment #21 by hsteoh — 2016-01-21T22:52:54Z
*** Issue 15567 has been marked as a duplicate of this issue. ***
In the discussion on the PR for this bug, #313 and #314 were mentioned as primary candidates to be fixed in the next compiler release. I think it makes sense to fix these module-related bugs as the goal for this release, since some of them will be breaking changes, and it's better to break module-related stuff once and have it work correctly from then on, than to be forced to break code multiple times -- users would revolt.
Comment #28 by timon.gehr — 2016-02-17T14:18:55Z
(In reply to hsteoh from comment #25)
> Woohoo! Finally this bug is fixed! Big thanks to Walter for doing this.
>
It's probably not the best fix though. Now locally imported symbols can still hide symbols imported in a less deeply nested scope, hence hijacking is still not prevented. This can also be dangerous when refactoring a large module into smaller ones.
Comment #29 by hsteoh — 2016-02-17T18:27:33Z
Do you have an example where this is problematic?
At any rate, it's better than the stonewall silence we've had on import issues for many years now.
Comment #30 by timon.gehr — 2016-02-17T19:53:01Z
(In reply to hsteoh from comment #29)
> Do you have an example where this is problematic?
>
Slightly contrived, but here you go:
import std.stdio;
string readAndLog(string filename){
import std.file;
auto text=readText(filename);
write(filename," read successfully!\n");
return text;
}
void main(){
writeln(readAndLog("important_data.txt"));
}
> At any rate, it's better than the stonewall silence we've had on import
> issues for many years now.
I agree. This is way better than what we had before. If a fix for hijacking is implemented, I would personally prefer to pick a proposed solution that actually fixes the hijacking problems.
Comment #31 by hsteoh — 2016-02-17T21:04:16Z
Ouch. That's a pretty nasty one. I suppose it's because the inner import pulls in 'write' into a different overload set from the outer import, so the compiler doesn't complain about the ambiguity error, even though the user did not intend to call std.file.write (rather than std.stdio.write) here.
So unqualified local imports are still extremely dangerous... :-(
Comment #32 by schveiguy — 2016-02-18T19:13:45Z
(In reply to hsteoh from comment #31)
> So unqualified local imports are still extremely dangerous... :-(
I think we should point out wherever the docs are that say how local imports work that best practice is to use selective or renamed imports as local imports to avoid issues.
And then fix Phobos/Druntime to follow those recommendations :)
Comment #33 by hsteoh — 2016-02-18T19:18:19Z
There has been a steady stream of PRs, at least when I was active last year, that moved imports into local scope (to keep Phobos internal interdependencies under control) and turned local imports into scoped imports (to prevent hijacking). If you find any other places in Phobos that has possibly-hijackable local imports, please file a bug or submit a PR. :-)
Comment #34 by schveiguy — 2016-02-18T19:23:53Z
What I mean is, really Timon's code should be:
string readAndLog(string filename){
import std.file : readText;
auto text=readText(filename);
write(filename," read successfully!\n");
return text;
}
i.e. only use scoped imports with a qualified list of which symbols you will use, or use a renamed import.
This prevents any hijacking.
Comment #35 by timon.gehr — 2016-02-18T19:49:33Z
(In reply to Steven Schveighoffer from comment #34)
> What I mean is, really Timon's code should be:
>
> string readAndLog(string filename){
> import std.file : readText;
> auto text=readText(filename);
> write(filename," read successfully!\n");
> return text;
> }
>
> i.e. only use scoped imports with a qualified list of which symbols you will
> use, or use a renamed import.
>
> This prevents any hijacking.
Well, there's an arbitrary number of conventions that will prevent any hijacking.
The point of my example was to expose a remaining flaw in the new import lookup design.
Comment #36 by dfj1esp02 — 2016-06-16T13:23:56Z
*** Issue 12279 has been marked as a duplicate of this issue. ***
Comment #37 by dlang-bugzilla — 2017-07-03T19:36:17Z
*** Issue 15819 has been marked as a duplicate of this issue. ***
Comment #38 by github-bugzilla — 2017-08-02T08:07:40Z
dlang/dmd pull request #10167 "Fixup date for removing check10378/bug10378 globals" was merged into master:
- 6da24d438a6b75ed0db8eb1a8b1f1b63318f19a6 by Geod24:
Fixup date for removing check10378/bug10378 globals
Introduced by 625c75a1b3.
https://github.com/dlang/dmd/pull/10167