Fix Issue 18219 - Private import inside struct leaks symbols when used as VarDeclaration types by RazvanN7 · Pull Request #7668 · dlang/dmd

RazvanN7 · 2018-01-10T10:23:20Z

No description provided.

dlang-bot · 2018-01-10T10:23:22Z

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	18219	Private import inside struct leaks symbols when used as VarDeclaration types

JinShil · 2018-01-10T10:27:41Z

src/dmd/dsymbol.d

     *      symbol found, NULL if not
     */
-    final Dsymbol searchX(Loc loc, Scope* sc, RootObject id)
+    final Dsymbol searchX(Loc loc, Scope* sc, RootObject id, int flags)


Please add Params DDoc comment.

Normally DAutotest should check that and fail respectively (except that we broke it today - #7637)

Is this ok?

Is this ok?

IMO, yes. Though I'm looking forward to replacing flags with a proper enum. But that's another PR.

wilzbach · 2018-01-10T10:32:47Z

Mind the CI failures:

dmd/parsetimevisitor.d(17): Error: no property 'Statement' for type 'ASTCodegen', did you mean 'dmd.statement.Statement'?
dmd/parsetimevisitor.d(19): Error: no property 'Expression' for type 'ASTCodegen', did you mean 'dmd.expression.Expression'?
dmd/parsetimevisitor.d(20): Error: no property 'TemplateParameter' for type 'ASTCodegen', did you mean 'dmd.dtemplate.TemplateParameter'?
dmd/parsetimevisitor.d(21): Error: no property 'Condition' for type 'ASTCodegen', did you mean 'dmd.cond.Condition'?
dmd/parsetimevisitor.d(22): Error: no property 'Initializer' for type 'ASTCodegen', did you mean 'dmd.init.Initializer'?
dmd/parsetimevisitor.d(26): Error: no property 'AliasThis' for type 'ASTCodegen', did you mean 'dmd.aliasthis.AliasThis'?
dmd/parsetimevisitor.d(27): Error: no property 'Declaration' for type 'ASTCodegen', did you mean 'dmd.declaration.Declaration'?
dmd/parsetimevisitor.d(28): Error: no property 'ScopeDsymbol' for type 'ASTCodegen', did you mean 'dmd.dsymbol.ScopeDsymbol'?
dmd/parsetimevisitor.d(29): Error: no property 'Import' for type 'ASTCodegen', did you mean 'dmd.dimport.Import'?
dmd/parsetimevisitor.d(30): Error: no property 'AttribDeclaration' for type 'ASTCodegen', did you mean 'dmd.attrib.AttribDeclaration'?
dmd/parsetimevisitor.d(31): Error: no property 'StaticAssert' for type 'ASTCodegen', did you mean 'dmd.staticassert.StaticAssert'?
dmd/parsetimevisitor.d(32): Error: no property 'DebugSymbol' for type 'ASTCodegen', did you mean 'dmd.dversion.DebugSymbol'?
dmd/parsetimevisitor.d(33): Error: no property 'VersionSymbol' for type 'ASTCodegen', did you mean 'dmd.dversion.VersionSymbol'?
dmd/parsetimevisitor.d(36): Error: no property 'Package' for type 'ASTCodegen', did you mean 'dmd.dmodule.Package'?
dmd/parsetimevisitor.d(37): Error: no property 'EnumDeclaration' for type 'ASTCodegen', did you mean 'dmd.denum.EnumDeclaration'?
dmd/parsetimevisitor.d(38): Error: no property 'AggregateDeclaration' for type 'ASTCodegen', did you mean 'dmd.aggregate.AggregateDeclaration'?
dmd/parsetimevisitor.d(39): Error: no property 'TemplateDeclaration' for type 'ASTCodegen', did you mean 'dmd.dtemplate.TemplateDeclaration'?
dmd/parsetimevisitor.d(40): Error: no property 'TemplateInstance' for type 'ASTCodegen', did you mean 'dmd.dtemplate.TemplateInstance'?
dmd/parsetimevisitor.d(41): Error: no property 'Nspace' for type 'ASTCodegen', did you mean 'dmd.nspace.Nspace'?
dmd/parsetimevisitor.d(45): Error: no property 'VarDeclaration' for type 'ASTCodegen', did you mean 'dmd.declaration.VarDeclaration'?

RazvanN7 · 2018-01-10T10:34:00Z

@wilzbach Yeah, I need to talk with @andralex about that.

wilzbach · 2018-01-10T10:37:33Z

Yeah, I need to talk with @andralex about that.

It looks similar to the bug found in dlang/phobos#5584. I suspect this will need to trigger a deprecation for a while.

Only meant to approve changes, not the entire PR.

RazvanN7 · 2018-01-10T11:07:23Z

After this is merged I sugest we rename searchX which does not say anything to searchTypeMembers
and move it to mtype.d since that is the only place where it is used. Later on, we should move all this functionality out of the AST nodes.

RazvanN7 · 2018-01-15T10:52:17Z

Circle/ci and cybershadow fail because the version of the compiler used to compile the compiler is still under the effect of the lookup bug that this PR fixes

wilzbach · 2018-01-15T15:36:53Z

still under the effect of the lookup bug that this PR fixes

Any way to work around it? You can bump CircleCi at .circleci/run.sh

WalterBright · 2018-01-16T01:48:20Z

src/dmd/mtype.d

            {
                if (s)
-                    error(loc, "no property `%s` for type `%s`, did you mean `%s`?", ident.toChars(), toChars(), s.toChars());
+                    error(loc, "no property '%s' for type '%s', did you mean '%s'?", ident.toChars(), toChars(), s.toPrettyChars());


Please don't revert the back ticks.

RazvanN7 · 2018-01-19T09:38:14Z

@andralex I transformed the error into a deprecation message.

…d as VarDeclaration types

WalterBright · 2018-12-13T11:04:47Z

@RazvanN7 this seems to have introduced a regression https://issues.dlang.org/show_bug.cgi?id=19103

dlang-bot added the Severity:Bug Fix label Jan 10, 2018

RazvanN7 force-pushed the Lookup branch from 44e002c to ce24a5b Compare January 10, 2018 10:26

JinShil suggested changes Jan 10, 2018

View reviewed changes

wilzbach added the Review:Needs Work label Jan 10, 2018

dlang-bot removed the Review:Needs Work label Jan 10, 2018

JinShil previously approved these changes Jan 10, 2018

View reviewed changes

dlang-bot added Review:Needs Work Review:Needs Rebase and removed Review:Needs Work Review:Needs Rebase labels Jan 10, 2018

RazvanN7 force-pushed the Lookup branch 2 times, most recently from 3f5bcbd to eb98708 Compare January 15, 2018 10:33

dlang-bot added the Review:Needs Work label Jan 15, 2018

wilzbach closed this Jan 15, 2018

wilzbach reopened this Jan 15, 2018

WalterBright reviewed Jan 16, 2018

View reviewed changes

dlang-bot removed the Review:Needs Work label Jan 19, 2018

RazvanN7 added 4 commits January 22, 2018 11:23

Fix Issue 18219 - Private import inside struct leaks symbols when use…

9bcb80a

…d as VarDeclaration types

Add ddoc style section for searchX

580c475

Fix symbol lookup to find public imports nested by structs

039cdf1

Transform error into a deprecation message

6d25a08

Add changelog entry

6decb34

RazvanN7 force-pushed the Lookup branch from dfb9075 to 6decb34 Compare January 22, 2018 09:24

andralex approved these changes Jan 22, 2018

View reviewed changes

andralex merged commit 1018db0 into dlang:master Jan 22, 2018

wilzbach mentioned this pull request Jan 23, 2018

Remove visiblity and lookup deprecation #7241

Closed

RazvanN7 mentioned this pull request Jul 3, 2018

Delete useless aliases in astcodegen.d #8445

Closed

Uh oh!

Comments

Conversation

RazvanN7 commented Jan 10, 2018

Uh oh!

dlang-bot commented Jan 10, 2018

Bugzilla references

Uh oh!

JinShil Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

JinShil Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jan 10, 2018

Uh oh!

RazvanN7 commented Jan 10, 2018

Uh oh!

wilzbach commented Jan 10, 2018

Uh oh!

RazvanN7 commented Jan 10, 2018

Uh oh!

RazvanN7 commented Jan 15, 2018

Uh oh!

wilzbach commented Jan 15, 2018

Uh oh!

WalterBright Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Jan 19, 2018

Uh oh!

WalterBright commented Dec 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants