Fix issue 313 & 314 implement more strict import mechanism

[9rnsr]

Issue 313 - [module] Fully qualified names bypass private imports
Issue 314 - [module] Static, renamed, and selective imports are always public

Changed points:

• Add package table in each scope, due to check which package/module name is accessible via imports.
• Renamed/Selective imports no longer create aliases for the introduced names.
• Module Scope Operator with fully qualified name no longer works.

[ghost]

Module Scope Operator with fully qualified name no longer works.

Could you show the code example for this?

[9rnsr]

Could you show the code example for this?

dlang/phobos@b7216ea#L3R104

[ghost]

dlang/phobos@b7216ea#L3R104

What if you want to disambiguate between a local symbol and an import?

import bar;

void main()
{
    struct Bar { void foo() { } }
    auto bar = Bar();

    .bar.foo();  // don't call instance, but a global foo function
}

I don't think it's a good idea to start breaking code like this.

[9rnsr]

You can use renamed import for the case.

Essentially, module scope operator is for accessing names declared in "module scope level". However, in your case, the 'bar' is a package name, and it is not a member of the module.

module foo;
import bar;
void main() {
    .bar.baz();
    // means accessing a 'bar' _declared in the module level scope of 'foo'_.
    // So inherently it is equivalent with 'foo.bar.baz()'
}

The behavior was the unintended behavior which had came from wrong compiler implementation. We should not keep it.

[leandro-lucarella-sociomantic]

On Tue, Jun 25, 2013 at 06:38:17AM -0700, Hara Kenji wrote:

You can use renamed import for the case.

Essentially, module scope operator is for accessing names declared in "module scope level". However, in your case, the 'bar' is a package name, and it is not a member of the module.

module foo;
import bar;
void main() {
    .bar.baz();
    // means accessing a 'bar' _declared in the module level scope of 'foo'_.
    // So inherently it is equivalent with 'foo.bar.baz()'
}

The behavior was the unintended behavior which had came from wrong compiler implementation. We should not keep it.

For me is not that clear that "module scope level" mean only stuff
declared in the module. bar (the module) is the module scope, not in
main(). If supporting the "old" use of "." is not hurting anyone (I
think the example shown above is pretty clear), why would you want to
disable it? It has very very low ROI IMHO. Is basically breaking code
for free (or is there any problem with it?).

[9rnsr]

@leandro-lucarella-sociomantic Because there's no consistent semantics to support it properly. It's completely wrong behavior which dependent on current dmd front-end implementation. Keeping non-descriptive behavior will stop writing another D front end.

[leandro-lucarella-sociomantic]

On Tue, Jun 25, 2013 at 08:03:32AM -0700, Hara Kenji wrote:

@leandro-lucarella-sociomantic Because there's no consistent semantics to support it properly. It's completely wrong behavior which dependent on current dmd front-end implementation. Keeping non-descriptive behavior will stop writing another D front end.

What do you mean by "no consistent semantics to support it properly"? I
still don't see why is it wrong from a user POV, I don't know if you are
talking about the implementation or the language design.

[9rnsr]

For example, currently dmd accepts this:

import std.stdio;
void main() { .std.stdio.writeln(); }

However this is rejected.

void main() {
    import std.stdio;
    .std.stdio.writeln();
}

One another case. If a module 'a' exists:

module a;
static import std.stdio;

Current dmd accepts this:

module b;
import a;
void main() { .std.stdio.writeln(); }

But rejects:

module b;
static import a;
void main() { .std.stdio.writeln(); }

I couldn't look any "consistent" semantics there.
Finally I concluded that it's just an "accepts-invalid" bug. We should just drop it.

[ghost]

All of these samples you posted make sense to me. I've always looked at . to mean the compiler should look in the most outer scope for a symbol name, in other words writing .foo would be the equivalent of physically actually being in the module scope, e.g.:

// #2 here

void foo()
{
    .bar(); // #1 pretend we're at location #2, and look for 'bar' there
}

[leandro-lucarella-sociomantic]

The fact that the current state of the compiler is broken it doesn't mean we can't give it a consistent semantic. I also had the same semantic @AndrejMitrovic mentioned in mind, and I think is consistent and very useful. A case where I used it is to wrap C libraries, a very common pattern is (extremely simplified):

import std.stdc.posix.unistd;

class File
{
    int read(size_t bytes)
    {
         return .read(bytes):
    }
}

Is not that I couldn't live with it, what I'm saying is this used to work, and is perfectly fine and clear what the intention is, so why should we break it? Again, if keeping the right semantics is extremely hard implementation-wise, I can understand your desire to drop it, but so far you never said it was an implementation problem.

[9rnsr]

@leandro-lucarella-sociomantic This change does not break your case. In module scope, read function is imported from std.stdc.posix.unistd module, and it would be accessible by .read.

import std.stdc.posix.unistd;

class File
{
    int read(size_t bytes)
    {
         return .read(bytes):    // still OK
         return std.stdc.posix.unistd.read(bytes):    // still OK
         return .std.stdc.posix.unistd.read(bytes):    // Disabled by this change
    }
}

Note the thing disabled by this is: "Module Scope Operator with fully qualified name".

[leandro-lucarella-sociomantic]

On Tue, Jun 25, 2013 at 09:26:07AM -0700, Hara Kenji wrote:

@leandro-lucarella-sociomantic This change does not break your case. In module scope, read function is imported from std.stdc.posix.unistd module, and it would be accessible by .read.

import std.stdc.posix.unistd;

class File
{
    int read(size_t bytes)
    {
         return .read(bytes):    // still OK
         return std.stdc.posix.unistd.read(bytes):    // still OK
         return .std.stdc.posix.unistd.read(bytes):    // Disabled by this change
    }
}

Note the thing disabled by this is: "Module Scope Operator with fully qualified name".

OK, please ignore my comments then, that seems to be an extreme corner
case and seems reasonable to make it go away if is convenient (I still
think it should stay if it's easy to implement though).

[donc]

I've changed my mind about this a couple of times.
@AndrejMitrovic's semantics seem to be the only ones that make sense, but they do mean that in the case of a local import, .std.stdio.writeln() will not compile. This is something you need to think about before it makes sense, and it will definitely confuse people initially. But it is consistent.
This currently compiles, and I think it should continue to do so.

struct std {
    struct stdio {
    static void writeln() {}
    }
}
void bar()  {
    .std.stdio.writeln();
}

This does not currently compile.

void bar()
{
    struct std {
        struct stdio {
        static void writeln() {}
        }
    }
    .std.stdio.writeln();
}

I think the import should behave in exactly the same way as the struct declaration. One indication that they should be the same is that you cannot have the import and the struct declaration in the same scope, otherwise you get an error like:

foo.d(3): Error: struct foo.std conflicts with import foo.std at foo.d(1)

[WalterBright]

I don't agree that the existing behavior is inconsistent or wrong. The leading '.' means lookup the name starting at module scope. All the examples follow the rules of that.

[9rnsr]

I think the import should behave in exactly the same way as the struct declaration.

@donc , I have completely opposite opinion. A package/module name which introduced by import and a name which declared in module should not conflict each other.

I'm understanding that the language semantic that you and today's dmd front-end would intend - that's just a single namespace shared by all symbols in a scope. I can agree that's a consistent semantic, but at the same time, I must say it would be unsuitable and difficult to understand. In other words, current intrusive import against the importing scope is bad. For example:

module foo;
void foo() {}
void test1() { foo(); } // prefer function name

module bar;
import foo;
void test2() { foo(); } // prefer module name - OOPS!

Currently, in module 'bar', import foo intrusively inserts a module name 'foo', instead of opening an additional search path to the declared symbols in foo. Then compiler rejects foo() in test2, and it would be unexpected for most programmers.

Instead, I think that import mechanism should only affect symbol visibility. When an idenfier foo is searched, import foo should search the member of imported module foo first. If it isn't found, import foo should return module foo as the found symbol.

After all, a properly declared symbol (aggregates, functions, templates, and variables) should be always preferred than package/module names in symbol searching. To do that, each scope should have two name spaces - one is for declared symbols in the scope, and another is for imported package/module names. This PR implements this by ScopeDsymbol::symtab and ScopeDsymbol::pkgtab.

[leandro-lucarella-sociomantic]

On Wed, Jun 26, 2013 at 02:46:55AM -0700, Hara Kenji wrote:

After all, a properly declared symbol (aggregates, functions, templates, and variables) should be always preferred than package/module names in symbol searching.

I don't agree with this. I think is less safe than making them clash.
Example:

import foo; // bar is also present here
void bar() {}
void f() { bar(); }

They you want to remove bar() from the module and forget to update f():

import foo; // bar is also present here
void f() { bar(); }

Now this is still compiling and calling foo.bar()!

[9rnsr]

@leandro-lucarella-sociomantic That's not a problem. When you import a module foo, you should recognize what symbol is actually imported.

[9rnsr]

@WalterBright Yes, current behavior is not a bug from the view of current implementation. BUT, that's very weird and hard to understand. Honestly I couldn't understand the correct import mechanism behavior until looking at dmd code. I believe that most programmers would imagine that "import" would provide a way to control symbol visibility. But contrary to that, current D's import is intrusive to the importing scope. If import is only allowed in module scope, it would had not been a problem. But, today D accepts function/class/struct local imports. Then the inserted name become local in those scope, and introduce broken behavior for access check and fully qualified name access.

import std.algorithm; // module scope import
class C { import std.stdio; }    // class scope import
void main() {
    import std.stdio;  // function local import
    .std.algorithm.map(a=>a)([1,2,3];  // currently OK
    .std.stdio.writeln();  // NG
    // -> what is the "fully qualified name"?

    C.std.stdio.writeln();  // currently OK
    // 'std' is package name, but it looks like declared in class C. Strange!
}

I argue that the current behavior is just debt anymore. In D1 age, it was reasonable for "easy-implementable language". However in D2, that's just an unacceptable behavior to me.

[9rnsr]

Updated commits for these issues.

• Issue 7491 - import symbol name unavailable in class scope
  Move original test case compilable/test7491.d to compilabl/testimport3.d, and add practical case in new compilable/test7491.d.
• Issue 10128 - import statement in base class members should be private by default

[ghost]

BUT, that's very weird and hard to understand.

What's weird and hard to understand? The whole point of the dot syntax is to disambiguate between local and module-scoped symbols. If you can't even do that anymore then the dot syntax completely loses it's purpose.

[9rnsr]

@AndrejMitrovic I have no ambiguity about the dot syntax (== Module Scope Operator) semantics. The thing I have been difficult to understand was why .std.stdio.writeln is accepted despite of the package name std is not a member of current module. Right now I know the actual reason by looking at dmd code, but the compiler behavior is still unacceptable as one of D users.

[ghost]

You've confused me a little bit with the samples (not knowing whether something is the current behavior or future behavior):

import std.algorithm;

void main()
{
    .std.stdio.writeln();  // currently compiles
}

I have no idea why that works now, I think it should fail.

As for this:

void main()
{
    struct S
    {
        import std.stdio;
    }

    S.std.stdio.writeln();
}

This currently compiles, but I don't think I've ever used it (I doubt anyone else has either). Local imports are a pretty recent feature. I don't have a strong opinion about this case.

With that being said, here is the current behavior I want to keep:

module test;

import std.stdio;

@disable void writeln() { }

void main()
{
    struct std
    {
        struct stdio
        {
            @disable static void writeln() { }
        }
    }

    .std.stdio.writeln();  // OK, grabs the phobos writeln function

    writeln();             // calls @disable function, fails compilation
    .writeln();            // ditto
    std.stdio.writeln();   // ditto (inner struct function)
}

[9rnsr]

import std.algorithm;
void main()
{
    .std.stdio.writeln();  // currently compiles
}

I have no idea why that works now, I think it should fail.

This PR will reject .std.stdio.writeln as you think.

S.std.stdio.writeln();

Yes, it also be rejected.

.std.stdio.writeln(); // OK, grabs the phobos writeln function

It would fail to compile, as same as the first case.

writeln();             // calls @disable function, fails compilation
.writeln();            // ditto
std.stdio.writeln();   // ditto (inner struct function)

Exactly.

[ghost]

.std.stdio.writeln(); // OK, grabs the phobos writeln function

It would fail to compile, as same as the first case.

Why would it fail? It's explicitly calling the writeln in the phobos library.

[9rnsr]

.std is Module Scope Operator (.) + package name (std), but the std package is not a member of test module. So it would be "undefined identifier 'std' in module 'test'" error.

[ghost]

Module Scope Operator -> The scope word there is significant. It means in the scope of the module, not in the actual module.

Look at the documentation:

$(H3 Module Scope Operator)

    Sometimes, it's necessary to override the usual lexical scoping rules
    to access a name hidden by a local name. This is done with the
    global scope operator, which is a leading $(SINGLEQUOTE .):

---------
int x;

int foo(int x)
{
  if (y)
    return x;  // returns foo.x, not global x
  else
    return .x; // returns global x
}
---------

    The leading $(SINGLEQUOTE .) means look up the name at the module scope level.

That last part says everything: look up the name at the module scope level. It doesn't say "look up the symbol defined in the module."

[9rnsr]

@AndrejMitrovic , thanks for explanation. OK, I was completely misunderstanding about that.
I updated PR and now .std.stdio.writeln() is properly accepted.

[ghost]

I think this should be parenthesized? if (imp && (prot == PROTpublic || prot == PROTprotected))

[9rnsr]

Nice catch! Will fix.

[WalterBright]

In the issue 12396 case, import S = b76; no longer inert an aliased name S in the module scope of test.d .

But that was the whole point of the design. It is not broken, and it is used.

[WalterBright]

In the issue 12396 case, import S = b76; no longer inert an aliased name S in the module scope of test.d . So, when the S is searched, the normally imported a76.S and the module b76 renamed to S are treated equally, then causes ambiguous error.

This is an additional breaking change. S.s means the same as b76.S, and there is no ambiguity.

[DmitryOlshansky]

@WalterBright the point was that selective import should make symbol visible, not add an extra alias that affects the overload set.

[WalterBright]

@blackwhale this is not a selective import, it's a renamed import. And its point absolutely is to create another name for it that exists in the current scope, and that is exactly what it's behavior is and is used for.

[leandro-lucarella-sociomantic]

On Tue, Mar 18, 2014 at 08:22:58AM -0700, Hara Kenji wrote:

@WalterBright The root of the all of "breaks" is only one:

Renamed/Selective imports no longer create aliases for the introduced names.

Selective and renamed imports no longer insert the selected/renamed names in the imported scope. In the implementation details, they were done by inserting implicitly created AliasDeclarations. Now,
instead of that, those names will be searched under the same name lookup rule with basic imports.

In the issue 12396 case, import S = b76; no longer inert an aliased name S in the module scope of test.d. So, when the S is searched, the normally imported a76.S and the module b76 renamed to S are treated equally, then causes ambiguous error.

I think this is one of those breaking changes people are willing to
accept, is probably one of the oldest and most controversial issues in
DMD history.

Having a clear and painless migration path will be highly appreciated
though.

[mihails-strasuns]

people are willing to accept

No.

[WalterBright]

I think this is one of those breaking changes people are willing to
accept,

No, because it broke code that has come from users, and I expect there's much more of that out there.

is probably one of the oldest and most controversial issues in DMD history.

313 and 314 are about respecting private declarations, they are not about changing the lookup rules.

I confess I don't even understand what the new rules are, let alone how to fix code so it works under the new rules. In the example I posted here, I don't know how to fix it for the new rules. I don't understand Kenji's explanation.

[WalterBright]

I suspect that even if I was wholly in favor of this PR, we would be forced to back it out if we tried to release this. People make complicated use of import declarations, and they are not at all going to be happy about having to reengineer them.

[leandro-lucarella-sociomantic]

On Wed, Mar 19, 2014 at 10:07:47AM -0700, Михаил Страшун wrote:

people are willing to accept

No.

I guess you are referring to the alias part of the change to
disambiguate symbols, I agree that is an undesired change, I'm referring
to all the other imports that need to be changed that were wrong in the
first place.

[WalterBright]

I'm referring to all the other imports that need to be changed that were wrong in the
first place.

Can you be more specific?

[leandro-lucarella-sociomantic]

On Wed, Mar 19, 2014 at 03:41:40PM -0700, Walter Bright wrote:

I'm referring to all the other imports that need to be changed that were wrong in the
first place.

Can you be more specific?

OK, maybe I'm misunderstanding the issue here, I'm just talking about
purely #313 and #314 examples:

---

// explicit private no longer needed but added for clarity

private import std.stdio;

In file b.d:

import a;

void main() {
// compiler correctly reports "undefined identifier writefln"
writefln("foo");
// works fine!
std.stdio.writefln("foo");

}

---

---

// explicit privates unnecessary but added for clarity
version(stat) private static import std.stdio;
version(rena) private import io = std.stdio;

version(sele) private import std.stdio : writefln;

In file b.d:

import a;

void main() {
version(stat) std.stdio.writefln("This should not work.");
version(rena) io.writefln("This should not work.");
version(sele) writefln("This should not work.");

}

[9rnsr]

I opened #3407.

[denis-sh]

Also this wrong code compiles with this (#2256) pull:

import io = std.stdio;

void main()
{
    writeln("hello!"); // ok, should be an error
}

[ghost]

I don't think that's wrong code. It's wrong code if you use static improt io = std.stdio;. Otherwise you're allowed to write either writeln or io.writeln, and with static you would only be able to write io.writeln.

[denis-sh]

I don't think that's wrong code.

Import declaration docs think it is (see "Renamed Imports").

[mihails-strasuns]

I am with @AndrejMitrovic here, looks more like bug in spec than in behavior. Otherwise static would have become no-op for renamed imports.

[dnadlinger]

I agree with Denis, to me it's natural that renamed imports don't add the raw symbols to the importing scope. After all, you use renamed imports precisely if you want to control the availability of symbols. Requiring the use of static just adds clutter in my opinion. And up to this pull request (or the last release, at least) the frontend indeed behaved like this.

[ghost]

And up to this pull request (or the last release, at least) the frontend indeed behaved like this.

I somehow thought it was existing behavior. Well, it could go either way as far as I'm concerned..

[9rnsr]

The spec doc was written based on the compiler implemented behavior. So it would not become the basis for accepting current behavior unconditionally.

Anyway, the new PRs #3407 and #3416 don't change it. Because finally I agreed that at least it should be changed by proper deprecation process.