Skip to content
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

Issue 15856 - Confusing error message with -transition=checkimports #5651

Merged
merged 4 commits into from
May 3, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/dclass.d
Original file line number Diff line number Diff line change
Expand Up @@ -1120,7 +1120,11 @@ public:
return null;
}

Dsymbol s = ScopeDsymbol.search(loc, ident, (flags & SearchImportsOnly) ? flags : flags | SearchLocalsOnly);
auto s = ScopeDsymbol.search(loc, ident, flags);

if ((flags & (SearchLocalsOnly | SearchImportsOnly)) != 0)
flags |= SearchLocalsOnly;

if (!s)
{
// Search bases classes in depth-first, left to right order
Expand All @@ -1135,7 +1139,7 @@ public:
{
import ddmd.access : symbolIsVisible;

s = b.sym.search(loc, ident, flags | SearchLocalsOnly);
s = b.sym.search(loc, ident, flags);
if (!s)
continue;
else if (s == this) // happens if s is nested in this and derives from this
Expand Down
3 changes: 3 additions & 0 deletions src/dmodule.d
Original file line number Diff line number Diff line change
Expand Up @@ -1093,10 +1093,13 @@ public:
// toChars(), ident->toChars(), flags, insearch, searchCacheSymbol ? searchCacheSymbol->toChars() : "null");
return searchCacheSymbol;
}

uint errors = global.errors;

insearch = 1;
Dsymbol s = ScopeDsymbol.search(loc, ident, flags);
insearch = 0;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unrelated

if (errors == global.errors)
{
// Bugzilla 10752: We can cache the result only when it does not cause
Expand Down
26 changes: 20 additions & 6 deletions src/dscope.d
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,6 @@ struct Scope
return sold;

// Search both ways
flags |= SearchCheck10378;
}

// First look in local scopes
Expand Down Expand Up @@ -554,17 +553,32 @@ struct Scope
{
alias snew = s;
if (sold !is snew)
{
deprecation(loc, "local import search method found %s %s instead of %s %s",
sold ? sold.kind() : "nothing", sold ? sold.toPrettyChars() : null,
snew ? snew.kind() : "nothing", snew ? snew.toPrettyChars() : null);
}
deprecation10378(loc, sold, snew);
if (global.params.bug10378)
s = sold;
}
return s;
}

/* A helper function to show deprecation message for new name lookup rule.
*/
extern (C++) static void deprecation10378(Loc loc, Dsymbol sold, Dsymbol snew)
{
OutBuffer buf;
buf.writestring("local import search method found ");
if (sold)
buf.printf("%s %s", sold.kind(), sold.toPrettyChars());
else
buf.writestring("nothing");
buf.writestring(" instead of ");
if (snew)
buf.printf("%s %s", snew.kind(), snew.toPrettyChars());
else
buf.writestring("nothing");

deprecation(loc, buf.peekString());
}

extern (C++) Dsymbol search_correct(Identifier ident)
{
if (global.gag)
Expand Down
12 changes: 0 additions & 12 deletions src/dsymbol.d
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,6 @@ enum : int
// meaning don't search imports in that scope,
// because qualified module searches search
// their imports
SearchCheck10378 = 0x40, // unqualified search with transition=checkimports switch
IgnoreSymbolVisibility = 0x80, // also find private and package protected symbols
}

Expand Down Expand Up @@ -1276,9 +1275,6 @@ public:
//printf("%s.ScopeDsymbol::search(ident='%s', flags=x%x)\n", toChars(), ident.toChars(), flags);
//if (strcmp(ident->toChars(),"c") == 0) *(char*)0=0;

if (global.params.bug10378 && !(flags & SearchCheck10378))
flags &= ~(SearchImportsOnly | SearchLocalsOnly);

// Look in symbols declared in this module
if (symtab && !(flags & SearchImportsOnly))
{
Expand Down Expand Up @@ -1311,15 +1307,7 @@ public:
if (ss.isModule())
{
if (flags & SearchLocalsOnly)
{
if (global.params.check10378 && !(flags & SearchCheck10378))
{
auto s3 = ss.search(loc, ident, sflags | IgnorePrivateImports);
if (s3)
deprecation("%s %s found in local import", s3.kind(), s3.toPrettyChars());
}
continue;
}
}
else
{
Expand Down
1 change: 0 additions & 1 deletion src/dsymbol.h
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ enum
// meaning don't search imports in that scope,
// because qualified module searches search
// their imports
SearchCheck10378 = 0x40, // unqualified search with transition=checkimports switch
IgnoreSymbolVisibility = 0x80, // also find private and package protected symbols
};

Expand Down
9 changes: 1 addition & 8 deletions src/expression.d
Original file line number Diff line number Diff line change
Expand Up @@ -679,9 +679,6 @@ extern (C++) Expression searchUFCS(Scope* sc, UnaExp ue, Identifier ident)
s = sold;
goto Lsearchdone;
}

// Search both ways
flags |= SearchCheck10378;
}

// First look in local scopes
Expand Down Expand Up @@ -709,11 +706,7 @@ extern (C++) Expression searchUFCS(Scope* sc, UnaExp ue, Identifier ident)
{
alias snew = s;
if (sold !is snew)
{
deprecation(loc, "local import search method found %s %s instead of %s %s",
sold ? sold.kind() : "nothing", sold ? sold.toPrettyChars() : null,
snew ? snew.kind() : "nothing", snew ? snew.toPrettyChars() : null);
}
Scope.deprecation10378(loc, sold, snew);
if (global.params.bug10378)
s = sold;
}
Expand Down
54 changes: 51 additions & 3 deletions src/mtype.d
Original file line number Diff line number Diff line change
Expand Up @@ -7868,14 +7868,38 @@ public:
// goto L1;
}
}
s = sym.search(e.loc, ident);

Copy link
Member

@MartinNowak MartinNowak May 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we should get rid of the default flags for Dsymbol.search. They even differ for Dsymbol and ScopeDSymbol :o.

Dsymbol searchSym()
{
int flags = 0;
Dsymbol sold = void;
if (global.params.bug10378 || global.params.check10378)
{
sold = sym.search(e.loc, ident, flags);
if (!global.params.check10378)
return sold;
}

auto s = sym.search(e.loc, ident, flags | SearchLocalsOnly);
if (global.params.check10378)
{
alias snew = s;
if (sold !is snew)
Scope.deprecation10378(e.loc, sold, snew);
if (global.params.bug10378)
s = sold;
}
return s;
}

s = searchSym();
L1:
if (!s)
{
if (sym._scope) // it's a fwd ref, maybe we can resolve it
{
sym.semantic(null);
s = sym.search(e.loc, ident);
s = searchSym();
}
if (!s)
return noMember(sc, e, ident, flag);
Expand Down Expand Up @@ -8639,7 +8663,31 @@ public:
sc2.pop();
return e;
}
s = sym.search(e.loc, ident);

Dsymbol searchSym()
{
int flags = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to declare flags here, just use 0 and SearchLocalsOnly directly.

Dsymbol sold = void;
if (global.params.bug10378 || global.params.check10378)
{
sold = sym.search(e.loc, ident, flags);
if (!global.params.check10378)
return sold;
}

auto s = sym.search(e.loc, ident, flags | SearchLocalsOnly);
if (global.params.check10378)
{
alias snew = s;
if (sold !is snew)
Scope.deprecation10378(e.loc, sold, snew);
if (global.params.bug10378)
s = sold;
}
return s;
}

s = searchSym();
L1:
if (!s)
{
Expand Down
1 change: 1 addition & 0 deletions src/scope.h
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ struct Scope
Module *instantiatingModule();

Dsymbol *search(Loc loc, Identifier *ident, Dsymbol **pscopesym, int flags = IgnoreNone);
static void deprecation10378(Loc loc, Dsymbol *sold, Dsymbol *snew);
Dsymbol *search_correct(Identifier *ident);
Dsymbol *insert(Dsymbol *s);

Expand Down
3 changes: 3 additions & 0 deletions test/compilable/imports/a15856.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module imports.a15856;

alias int c_long;
17 changes: 17 additions & 0 deletions test/compilable/test15856.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// REQUIRED_ARGS: -transition=checkimports -de
// PERMUTE_ARGS:
/*
TEST_PUTPUT:
---
---
*/

class Foo
{
import imports.a15856;

struct Bar
{
c_long a;
}
}
16 changes: 7 additions & 9 deletions test/fail_compilation/checkimports2a.d
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,13 @@
/*
TEST_OUTPUT:
---
fail_compilation/checkimports2a.d(23): Deprecation: class checkimports2a.B variable imports.imp2.Y found in local import
fail_compilation/checkimports2a.d(27): Deprecation: local import search method found variable imports.imp1.Y instead of variable imports.imp2.Y
fail_compilation/checkimports2a.d(23): Deprecation: class checkimports2a.B variable imports.imp2.X found in local import
fail_compilation/checkimports2a.d(23): Deprecation: class checkimports2a.B variable imports.imp2.X found in local import
fail_compilation/checkimports2a.d(32): Error: no property 'X' for type 'checkimports2a.B'
fail_compilation/checkimports2a.d(23): Deprecation: class checkimports2a.B variable imports.imp2.Y found in local import
fail_compilation/checkimports2a.d(33): Error: no property 'Y' for type 'checkimports2a.B'
fail_compilation/checkimports2a.d(23): Deprecation: class checkimports2a.B variable imports.imp2.X found in local import
fail_compilation/checkimports2a.d(23): Deprecation: class checkimports2a.B variable imports.imp2.Y found in local import
fail_compilation/checkimports2a.d(24): Deprecation: local import search method found variable imports.imp2.X instead of variable checkimports2a.X
fail_compilation/checkimports2a.d(30): Deprecation: local import search method found variable imports.imp2.X instead of nothing
fail_compilation/checkimports2a.d(30): Error: no property 'X' for type 'checkimports2a.B'
fail_compilation/checkimports2a.d(31): Deprecation: local import search method found variable imports.imp2.Y instead of nothing
fail_compilation/checkimports2a.d(31): Error: no property 'Y' for type 'checkimports2a.B'
fail_compilation/checkimports2a.d(33): Deprecation: local import search method found variable imports.imp2.X instead of variable checkimports2a.X
fail_compilation/checkimports2a.d(34): Deprecation: local import search method found variable imports.imp2.Y instead of variable imports.imp1.Y
---
*/

Expand Down
8 changes: 5 additions & 3 deletions test/fail_compilation/checkimports2b.d
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
/*
TEST_OUTPUT:
---
fail_compilation/checkimports2b.d(20): Deprecation: local import search method found variable imports.imp2.X instead of variable checkimports2b.X
fail_compilation/checkimports2b.d(29): Deprecation: local import search method found variable imports.imp2.X instead of variable checkimports2b.X
fail_compilation/checkimports2b.d(30): Deprecation: local import search method found variable imports.imp2.Y instead of variable imports.imp1.Y
fail_compilation/checkimports2b.d(22): Deprecation: local import search method found variable imports.imp2.X instead of variable checkimports2b.X
fail_compilation/checkimports2b.d(28): Deprecation: local import search method found variable imports.imp2.X instead of nothing
fail_compilation/checkimports2b.d(29): Deprecation: local import search method found variable imports.imp2.Y instead of nothing
fail_compilation/checkimports2b.d(31): Deprecation: local import search method found variable imports.imp2.X instead of variable checkimports2b.X
fail_compilation/checkimports2b.d(32): Deprecation: local import search method found variable imports.imp2.Y instead of variable imports.imp1.Y
---
*/

Expand Down
8 changes: 5 additions & 3 deletions test/fail_compilation/checkimports2c.d
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
/*
TEST_OUTPUT:
---
fail_compilation/checkimports2c.d(20): Deprecation: local import search method found variable imports.imp2.X instead of variable checkimports2c.X
fail_compilation/checkimports2c.d(29): Deprecation: local import search method found variable imports.imp2.X instead of variable checkimports2c.X
fail_compilation/checkimports2c.d(30): Deprecation: local import search method found variable imports.imp2.Y instead of variable imports.imp1.Y
fail_compilation/checkimports2c.d(22): Deprecation: local import search method found variable imports.imp2.X instead of variable checkimports2c.X
fail_compilation/checkimports2c.d(28): Deprecation: local import search method found variable imports.imp2.X instead of nothing
fail_compilation/checkimports2c.d(29): Deprecation: local import search method found variable imports.imp2.Y instead of nothing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the test case, those don't seem very clear.
Could we have something that isn't refering to the compiler internal, e.g:
Deprecation: Variable imports.imp2.Y introduced by a private local import is not visible with the new import rule, use a public import or an alias ?

Other suggestions welcome, but "found X instead of nothing" is not user friendly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know through which import we found the symbol, and it would be too much work to figure out.

fail_compilation/checkimports2c.d(31): Deprecation: local import search method found variable imports.imp2.X instead of variable checkimports2c.X
fail_compilation/checkimports2c.d(32): Deprecation: local import search method found variable imports.imp2.Y instead of variable imports.imp1.Y
---
*/

Expand Down