New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Issue 10378 - Local imports hide local symbols #5445

Merged
merged 1 commit into from Feb 16, 2016

Conversation

Projects
None yet
@WalterBright
Member

WalterBright commented Feb 12, 2016

This is a fix of https://issues.dlang.org/show_bug.cgi?id=10378 and a reboot of #4915

It requires dlang/phobos#3989

This version is based on a comment Andrei made to me a long time ago about it. Symbol lookup is now done in two passes (formerly one). The first pass goes up through the scopes, checking everything but imported modules. If that fails, the scopes are gone through again, checking only imported modules.

This algorithm:

  1. retains the scoped nature of nested imports, i.e. names in nested imports will hide outer imported names
  2. works the same for function locals as it does for struct members, which was my complaint about #4915
  3. does not issue errors for shadowing - it simply finds the shadowed symbol first.
  4. breaks existing code. Turns out that people did rely on it. See dlang/phobos#3989
  5. is pretty simple. (the large number of lines of code changed is mostly due to debugging and printing code which I'll remove once it passes the autotester
  6. exposed a bug in the way the compiler is implemented, see dlang/phobos#3989 for that, too. I'm sure there'll be other instances in the wild of this.
  7. it should only be a hair slower, as it never looks in the same hash table twice
@dlang-bot

This comment has been minimized.

Contributor

dlang-bot commented Feb 12, 2016

Fix Bugzilla Description
10378 Prevent local imports from hiding local symbols

@WalterBright WalterBright force-pushed the WalterBright:fix10378-2 branch from 49441da to cf03be5 Feb 12, 2016

@WalterBright

This comment has been minimized.

Member

WalterBright commented Feb 12, 2016

Looks like @deadalnix deadalnix proposed this algorithm here:

https://issues.dlang.org/show_bug.cgi?id=10378#c9

{
Dsymbol sx = searchScopes(flags); // look in both locals and imports
version (LOGSEARCH)
{

This comment has been minimized.

@ibuclaw

ibuclaw Feb 12, 2016

Member

Remove version none and move initialising sx into version LOGSEARCH?

This comment has been minimized.

@WalterBright

WalterBright Feb 12, 2016

Member

that code will be removed when it passes the autotester

version (none)
if (s && sx != s)
{

This comment has been minimized.

@ibuclaw

ibuclaw Feb 12, 2016

Member

Cosmetic, but I'd prefer there to be braces here the version block.

This comment has been minimized.

@WalterBright

WalterBright Feb 12, 2016

Member

that code will be removed when it passes the autotester

This comment has been minimized.

@andralex

andralex Feb 12, 2016

Member

btw one nice thing:

version(whatevs) if (s && sx != x)
{
}

i.e. putting the two on the same line - clarifies that version and if are one item. So no need for two levels of indentation either.

@WalterBright WalterBright force-pushed the WalterBright:fix10378-2 branch from cf03be5 to d72662d Feb 12, 2016

@deadalnix

This comment has been minimized.

Contributor

deadalnix commented Feb 12, 2016

Yes, that's how it needs to be !

@klickverbot

This comment has been minimized.

Member

klickverbot commented Feb 12, 2016

We should make fixing this and issues 313/314 the main point of this release.

@WalterBright WalterBright force-pushed the WalterBright:fix10378-2 branch 6 times, most recently from 01374b7 to b293c4c Feb 12, 2016

@WalterBright WalterBright referenced this pull request Feb 12, 2016

Merged

fix import shadowing #3990

@WalterBright

This comment has been minimized.

Member

WalterBright commented Feb 12, 2016

Needs this Phobos fix:
dlang/phobos#3990

Folks, this language change is clearly going to break existing code.

@andralex

This comment has been minimized.

Member

andralex commented Feb 12, 2016

nice work!

@andralex

This comment has been minimized.

Member

andralex commented Feb 12, 2016

Auto-merge toggled on

@schveiguy

This comment has been minimized.

Member

schveiguy commented Feb 12, 2016

When this is merged, I think we need a doc change too.

@9rnsr

This comment has been minimized.

Member

9rnsr commented Feb 12, 2016

Auto-merge toggled off

@9rnsr

This comment has been minimized.

Member

9rnsr commented Feb 12, 2016

I want to review this.

@@ -0,0 +1,4 @@
module bar;

This comment has been minimized.

@9rnsr

9rnsr Feb 12, 2016

Member

A module bar; is imported by import imports.bar10378;. It's a mismatch.

I'm not sure why the error is not reported, but at least, this line should be: module imports.bar10378;.

This comment has been minimized.

@WalterBright

WalterBright Feb 12, 2016

Member

That's actually a feature of D. The file name doesn't have to match the module name.

This comment has been minimized.

@9rnsr

9rnsr Feb 13, 2016

Member

I know the feature, but this is not its case. This file is imported as the module name imports.bar10378, but the module declaration has a different fully qualified name bar which does not match.

This comment has been minimized.

@MartinNowak

MartinNowak Feb 13, 2016

Member

That's actually a feature of D. The file name doesn't have to match the module name.

It's actually the result of a horribly broken implementation. The module is inserted w/ both names into the global symbol table.

I'm not sure why the error is not reported

The error is only reported if the module is know under the module declaration name before importing it, this happens when it's parsed before the module importing it.

@@ -172,6 +172,8 @@ enum : int
IgnorePrivateMembers = 0x01, // don't find private members
IgnoreErrors = 0x02, // don't give error messages
IgnoreAmbiguous = 0x04, // return NULL if ambiguous
IgnoreImports = 0x08, // don't find symbols in imports
IgnoreThisModule = 0x10, // only look in imports

This comment has been minimized.

@9rnsr

9rnsr Feb 12, 2016

Member

dsymbol.h also needs to be updated.

This comment has been minimized.

@9rnsr

9rnsr Feb 12, 2016

Member

IgnoreThisModule is hard to understand what happens. IgnoreLocals or IgnoreLocalSymbol is better I think.

This comment has been minimized.

@9rnsr

9rnsr Feb 12, 2016

Member

Also, it needs to be commented that IgnoreImports and IgnoreThisModule are exclusive flags.

This comment has been minimized.

@WalterBright

WalterBright Feb 14, 2016

Member

dsymbol.h also needs to be updated.

There is no reason for the C++ code to ever need these values. Putting it in dsymbol.h would imply a reason.

This comment has been minimized.

@yebblies

yebblies Feb 14, 2016

Member

So dsymbol.h doesn't need any of the enum members? That's fine too, if they're all deleted. Having only half the values is messy.

return s;
// Stop when we hit a module, but keep going if that is not just under the global scope
if (sc.scopesym.isModule() && !(sc.enclosing && !sc.enclosing.enclosing))
break;

This comment has been minimized.

@9rnsr

9rnsr Feb 12, 2016

Member

Why this is newly added?

This comment has been minimized.

@WalterBright

WalterBright Feb 12, 2016

Member

Because there's a bug elsewhere in the compiler where modules are not at the top of the scope change. A followup to this will be where I find and fix that bug. In the Phobos corrections, std.zip had relied on that bug, which cost me many hours of hair pulling.

This comment has been minimized.

@9rnsr

9rnsr Feb 13, 2016

Member

So, this change is a bugfix and not directly related with the enhancement change for 10378, right? Please add a test case which hits this line, and add one more commit for that before the enhancement change inside this PR.

This comment has been minimized.

@WalterBright

WalterBright Feb 14, 2016

Member

It is directly related, as I discovered that this enhancement is not possible with it. But I was already deeply into it, and so simply worked around it here. As I said, once this is pulled I'll work on a followup to fix the scope chain correctly.

@9rnsr

This comment has been minimized.

Member

9rnsr commented Feb 12, 2016

@WalterBright : Also I give you a chance to remove debugging code from this PR.

@Geod24

This comment has been minimized.

Contributor

Geod24 commented Feb 12, 2016

@WalterBright : Is there any chance we get a deprecation for the old behaviour ?

I agree that is the right direction to go, and that old code relied on a bug. However, that code was not necessarily broken / buggy, which is why I don't think breaking it without a proper deprecation is okay.

@@ -1,3 +0,0 @@
module imports.diag12598a;
struct lines { }

This comment has been minimized.

@9rnsr

9rnsr Feb 12, 2016

Member

It's better to move to compilable directory, rather than deleting test case for issue 12598.

Of course adding a comment about that in commit log is even better.

This comment has been minimized.

@WalterBright

WalterBright Feb 12, 2016

Member

It doesn't test for anything novel anymore and serves no purpose. Hence, deleting it.

This comment has been minimized.

@9rnsr

9rnsr Feb 13, 2016

Member

You should record into the git history what happened in this PR. Moving the test case to compiable would be the best clarify way that the issue 12598 case is now just accepted.
If you don't like it, you should add a comment into the commit message, like "issue 12598 case is now properly accepted, so its test case fail_compilation/diag12598.d is deleted.

@9rnsr

This comment has been minimized.

Member

9rnsr commented Feb 12, 2016

The most of PR summary needs to be copied into commit log, no?

@9rnsr

This comment has been minimized.

Member

9rnsr commented Feb 12, 2016

Excepting my comments, the implementation itself looks good to me.

@WalterBright

This comment has been minimized.

Member

WalterBright commented Feb 16, 2016

313, yes, since it's ready to go. The rest, no, they are not ready. I intend to go back to work on EH, as it's clear it won't get done if I don't do it.

I don't agree with creating essentially a fork. We don't have a large enough community to make that worthwhile or manageable.

This is a good enhancement. There's a switch to revert to the old behavior. I think the worries about this, while I agree with the sentiment, are a bit overblown. Every case the new behavior errored on was, in my not so humble opinion, crap code to begin with and I was happy to fix them.

This enhancement is important because it is an enabling enhancement. It enables us to advocate, with confidence, the notion of using local imports to improve encapsulation and comprehensibility of code, which are key features of D. It resolves a problem that has been embarrassing for us, and greatly annoyed many people. I don't believe we should be sitting on it.

@WalterBright

This comment has been minimized.

Member

WalterBright commented Feb 16, 2016

add another switch

Please, no:

  1. complexity
  2. documentation
  3. it is not as easy as you suggest - note that every override of search() changed the flags.
  4. if a user sees new "undeclared symbol" messages, he can simply add the -transition=import switch and if the error goes away, he knows that's what the problem is.

BTW, doing a fork has more problems. This change touches a lot of files, and I've had to rebase it twice in 4 days just so it will merge. I'm not keen on the time investment in this.

@schveiguy

This comment has been minimized.

Member

schveiguy commented Feb 16, 2016

I don't have much to offer here, except support for this change. As much as this will break code, people have been clamoring for import issues to be fixed for almost a decade. I think folks will understand this is going to help them even though their code breaks.

Regarding build issues, I think those are somewhat secondary to the ability to have code that compiles for both older and newer versions of the compiler. Simply because the two alternative compilers lag behind DMD. So as long as there is a way to fix broken code so it compiles for the new scheme and the old scheme, I'd say passing the right flags is a secondary concern.

@MartinNowak

This comment has been minimized.

Member

MartinNowak commented Feb 16, 2016

Your arguments are correct, the very few issues this caused ¹ relied on rather obscure behavior, and the new lookup rules are a huge improvement.
The code is also clearer now, and we have a nice explanation in the changelog.

@MartinNowak

This comment has been minimized.

Member

MartinNowak commented Feb 16, 2016

Auto-merge toggled on

@JackStouffer

This comment has been minimized.

Member

JackStouffer commented Feb 16, 2016

Auto-merge toggled on

Thanks Walter for doing this!

@schveiguy

This comment has been minimized.

Member

schveiguy commented Feb 16, 2016

I have a question on this. If I want to have an inner scope where I add an overload via an import, how would I do it? Currently, you have to re-import the original so they are on the same level. From my reading of the new rules, this would continue to be the case.

An example of what I'm talking about is in the bug report this PR is fixing (at the end of the comment): https://issues.dlang.org/show_bug.cgi?id=10378#c16

MartinNowak added a commit that referenced this pull request Feb 16, 2016

Merge pull request #5445 from WalterBright/fix10378-2
fix Issue 10378 - Local imports hide local symbols

@MartinNowak MartinNowak merged commit 57592cf into dlang:master Feb 16, 2016

3 checks passed

CyberShadow/DAutoTest Documentation OK (no changes)
Details
auto-tester Pass: 10
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@WalterBright

This comment has been minimized.

Member

WalterBright commented Feb 17, 2016

Currently, you have to re-import the original so they are on the same level.

That's right (and this PR does not change that behavior).

Edit: Functions that exist in two imports do not actually overload against each other. They form separate "overload sets". The anti-hijacking rules keep them distinct. The only way you'd want to overload against both is if the scope wants to actually call both, in which case it makes sense to require re-importing just for clarity.

@WalterBright

This comment has been minimized.

Member

WalterBright commented Feb 17, 2016

So as long as there is a way to fix broken code so it compiles for the new scheme and the old scheme,

I had no problems doing that for the Phobos library breakage.

@schveiguy

This comment has been minimized.

Member

schveiguy commented Feb 17, 2016

Right. The only confusing part is to require importing the current module you are in (because another module hijacked a module symbol you were using).

Interestingly, this doesn't happen when you import at module level. The current module trumps any imports. But when you import at a scope, then the rules change. As a user of library code, I can see value in importing a module locally when it only pertains to that particular function. What I'm concerned with (and am still, even though I like the PR as merged), is that a movement of an import from the module level to a scoped level changes what happens.

example:

module a;

void foo() {import std.stdio; writeln("a");}

void bar() {}
module b;
import a;

void foo() {import std.stdio; writeln("b");}

void main()
{
    bar();
    foo();
}

This outputs "b"

Hey, look, we can just import a locally in main, because we only need bar in there!

module b;

void foo() {import std.stdio; writeln("b");}

void main()
{
    import a;
    bar();
    foo();
}

This now outputs "a".

I'm unsure of the "fix" for this, but it may be good practice to recommend always importing within an inner scope using renamed imports (to avoid hijacking like this).

@WalterBright WalterBright deleted the WalterBright:fix10378-2 branch Feb 17, 2016

@WalterBright

This comment has been minimized.

Member

WalterBright commented Feb 17, 2016

@schveiguy Your second example now outputs b with this PR. This fix is the whole point of this PR.

@schveiguy

This comment has been minimized.

Member

schveiguy commented Feb 17, 2016

our second example now outputs b with this PR

Oh right! I wasn't thinking about the fact that local module functions aren't actually imported :)

So no need to reimport the current module if the current module's function is what you want. This is great! Thanks.

@CyberShadow

This comment has been minimized.

Member

CyberShadow commented Mar 7, 2016

Could someone please tell me if this breakage is intentional or should I file a regression?

int memcmp(in ubyte[] a, in ubyte[] b)
{
    import core.stdc.string;
    assert(a.length == b.length);
    return memcmp(a.ptr, b.ptr, a.length);
}

This no longer works because it tries to call itself - not what I would expect TBH, seems like a bug to me.

@schveiguy

This comment has been minimized.

Member

schveiguy commented Mar 7, 2016

I think it's intentional, as imports now are considered after local module functions.

I disagree that it's a bug. If you imported core.stdc.string in the module, the imported memcmp wouldn't be considered.

@CyberShadow

This comment has been minimized.

Member

CyberShadow commented Mar 7, 2016

If you imported core.stdc.string in the module, the imported memcmp wouldn't be considered.

That would not be surprising. The behavior in the above snippet is surprising, though, because the import is "closer" (in scope terms) to the use of the memcmp symbol than the function declaration.

Changing the import to a selective import makes it compile again, which is weird, as I always thought that selective imports are simply selective versions of imports, and do not grant additional visibility to the selected symbols.

I kind of see why this change was necessary, though, but the additional complexity and code breakage isn't nice.

@schveiguy

This comment has been minimized.

Member

schveiguy commented Mar 7, 2016

Yes, selective import actually aliases the symbol (and only the one you selected) into your local scope, which would override any other imported symbols and local symbols.

It is going to be very confusing, and code-breaky, because it's been this way for so long. I don't think there's a way we could do this without breaking code.

@schveiguy

This comment has been minimized.

Member

schveiguy commented Mar 7, 2016

Note: I always looked at it the opposite way -- that importing in a local scope should not change how the import was done, just what scopes could see the import. Moving an import inside a function shouldn't change the behavior of the function, you are just trying to avoid polluting namespaces (you can see my example above).

@ibuclaw

This comment has been minimized.

Member

ibuclaw commented Mar 8, 2016

@schveiguy are you sure? core.stdc.string.memcmp has a different signature to the function that imports it's module. I can see no reason why that snippet would even consider recursively calling itself.

@WalterBright

This comment has been minimized.

Member

WalterBright commented Mar 8, 2016

Could someone please tell me if this breakage is intentional or should I file a regression?

It's behaving exactly as intended.

has a different signature

Signatures are considered for overload resolution, not for name lookup. D has always worked this way, and so has C++.

@MartinNowak

This comment has been minimized.

Member

MartinNowak commented Mar 9, 2016

Turn on -transition=checkimports, it should give you a proper deprecation warning about the changed lookup.

@CyberShadow

This comment has been minimized.

Member

CyberShadow commented Mar 10, 2016

Turn on -transition=checkimports, it should give you a proper deprecation warning about the changed lookup.

I am seeing new deprecation warnings even without this flag (using dmd master). Is the flag meant for stable and the breaking changes for master, or something else?

@CyberShadow

This comment has been minimized.

Member

CyberShadow commented Mar 10, 2016

It's behaving exactly as intended.

OK. Another question: does a selective import not imply a static one? E.g.:

import std.stdio : readln;
import std.conv;

void main()
{
    std.stdio.writeln("Hello, world!");
}

This now gives:

test.d(6): Deprecation: module std.stdio is not accessible here, perhaps add 'static import std.stdio;'

but that strikes me as redundant considering the selective import.

@schveiguy

This comment has been minimized.

Member

schveiguy commented Mar 10, 2016

does a selective import not imply a static one?

No it was not supposed to. But this is due to a different pull:

#5426

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