Remove visiblity and lookup deprecation (Take 2) by JinShil · Pull Request #9636 · dlang/dmd

JinShil · 2019-04-17T14:35:41Z

~~** This is not ready yet **~~

dlang-bot · 2019-04-17T14:35:43Z

Thanks for your pull request and interest in making D better, @JinShil! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9636"

thewilsonator · 2019-04-17T15:21:04Z

cc @atilaneves
https://github.com/kaleidicassociates/excel-d/blob/master/source/xlld/wrap/wrap.d#L604-L607

atilaneves · 2019-04-17T16:33:09Z

@thewilsonator Not entirely sure why I was cc'ed.

wilzbach · 2019-04-17T17:21:13Z

Because excel-d is failing with this PR:

source/xlld/wrap/wrap.d(607,24): Error: `test.d_funcs.shouldNotBeAProblem` is not visible from module `wrap`

jacob-carlborg · 2019-04-17T17:57:41Z

** This is not ready yet **

Next time try creating a draft PR: https://github.blog/2019-02-14-introducing-draft-pull-requests.

thewilsonator · 2019-04-17T18:23:50Z

Because excel-d is failing with this PR:

Yep. Sorry I should have put that in the ping.

JinShil · 2019-04-17T23:35:57Z

I believe all the failures in buildkite are legitimate. The only thing I have left (I think) is to improve some of the error messages and prevent duplicate error messages. In other words, I think the applications that are failing need to be fixed.

Geod24

Thanks for doing this. A few comments on error messages, but looks like you're still working on it.

test/fail_compilation/test143.d

test/fail_compilation/fail313.d

test/fail_compilation/fail262.d

Geod24 · 2019-04-18T02:06:04Z

test/fail_compilation/fail18979.d

 ---
-fail_compilation/fail18979.d(13): Deprecation: `imports.imp18979.Foo.__ctor(A)(A a)` is not visible from module `fail18979`
---
+fail_compilation/fail18979.d(14): Error: `imports.imp18979.Foo.__ctor(A)(A a)` is not visible from module `fail18979`


Shouldn't it be an "undefined identifier" error ?
Also, in a perfect world, we shouldn't show "__ctor" to the user (or any symbol which is not defined in code), but that was already the case before.

~~How about now?~~

Edit: Actually I don't think the suggestion "did you mean imports.imp18979.Foo.__ctor(A)(A a)" shouldn't appear at all because it is private. I'll have to look into it.

test/compilable/checkimports1.d

Geod24 · 2019-04-18T23:06:41Z

src/dmd/access.d

-            return true;
-        }
+        // Access checks have been removed in favor of visibility checks.
+        // See https://github.com/dlang/DIPs/blob/master/DIPs/archive/DIP22.md


Shouldn't this whole function be reworked as a result ?
Not necessarily now tho.

Yes, I'm hoping I can delete the entire function when I'm done.

I couldn't get rid of it. It is called for some safety checks, specifically https://github.com/dlang/dmd/blob/master/test/fail_compilation/test18554.d

src/dmd/globals.d

src/dmd/mars.d

src/dmd/traits.d

src/dmd/typesem.d

test/fail_compilation/checkimports2.d

test/fail_compilation/diag5385.d

test/fail_compilation/dip22a.d

src/dmd/cli.d

wilzbach · 2019-04-19T07:00:11Z

dlang-bot update PR: Add private imports explicitly dlang-bot#229
Vibe.d update PR: Add private imports explicitly vibe-d/vibe.d#2304

wilzbach · 2019-04-19T07:33:53Z

The Azure failure is pretty weird:

"D:\a\1\s\generated\Windows\Release\Win32\dmd.exe" -conf= -g -m32mscoff -O -w -dip25 -I../druntime\import -unittest -c  -ofunittest2.obj std\range\package.d  std\range\primitives.d  std\range\interfaces.d
Error: out of memory

I guess we split up building Phobos even more for win32 :/

rainers · 2019-04-19T08:51:10Z

The Azure failure is pretty weird:
I guess we split up building Phobos even more for win32 :/

BTW: the autotester uses posix.mak to build the phobos unittests in parallel and per file compilation on windows, too. See target auto-tester-test.

atilaneves · 2019-04-19T09:03:46Z

I've fixed excel-d in the meanwhile.

wilzbach · 2019-04-19T09:05:38Z

The Azure failure is pretty weird:
I guess we split up building Phobos even more for win32 :/

BTW: the autotester uses posix.mak to build the phobos unittests in parallel and per file compilation on windows, too. See target auto-tester-test.

Yeah, but I guess the auto-tester machines have more memory?
Do you have a better idea/solution to fixing this out of memory error on Azure?

rainers · 2019-04-19T09:23:39Z

Yeah, but I guess the auto-tester machines have more memory?

Might be, but single file compilation will likely need less memory and is what posix.mak does. If parallel builds are a problem due to memory resources, it could be used with JOBS=1.

rainers · 2019-04-19T09:39:42Z

Do you have a better idea/solution to fixing this out of memory error on Azure?

This might also help: #9650

JinShil · 2019-04-20T10:11:51Z

I have done as much as I can with this. There is one bug remaining:

Given the following files:

├── fail313.d
└── imports
    ├── a313.d
    └── b313.d

module fail313;
import imports.a313;
void main()
{
    imports.b313.bug();
}

module imports.a313;
private import imports.b313;

module imports.b313;
void bug() { }

dmd fail313.d
fail313.d(b): Error: module `imports.b313` is not accessible here, perhaps add `static import imports.b313;`

The error is emitted from the call to checkAccess at the bottom of this code block:

dmd/src/dmd/expressionsem.d

Lines 10993 to 11008 in a147141

    
                   if (ie.sds.isModule() && ie.sds != sc._module) 
        
                       flags |= IgnorePrivateImports; 
        
                   if (sc.flags & SCOPE.ignoresymbolvisibility) 
        
                       flags |= IgnoreSymbolVisibility; 
        
                   Dsymbol s = ie.sds.search(exp.loc, exp.ident, flags); 
        
                   /* Check for visibility before resolving aliases because public 
        
                    * aliases to private symbols are public. 
        
                    */ 
        
                   if (s && !(sc.flags & SCOPE.ignoresymbolvisibility) && !symbolIsVisible(sc._module, s)) 
        
                   { 
        
                       s = null; 
        
                   } 
        
                   if (s) 
        
                   { 
        
                       if (auto p = s.isPackage()) 
        
                           checkAccess(exp.loc, sc, p);

The problem is it should have never gotten that far. The call to ie.sds.search() should not have returned imports.b313 or the call to symbolIsVisible should not have returned true (Preferrablly the former).

It's an existing bug in symbol lookup/visibility that is being masked by the now deprecated access checks. I need to fix that, and remove the call to checkAccess. I've been working on it for 2 days and can't figure it out. I could use some help.

SSoulaimane · 2019-04-22T02:21:20Z

@JinShil isn't removing checkAccess() out of the scope of this PR?

JinShil · 2019-04-22T02:33:07Z

isn't removing checkAccess() out of the scope of this PR?

No, I don't believe so. When DIP22 was implemented, it had to keep both visibility checks and access checks working until the deprecation of access checks (this PR) was finished. With visibility checks implemented, and access checks now fully deprecated, access checks are no longer needed.

The problem, as the bug above illustrates, is that the visibility checks were not fully implemented; the original authors missed the one case illustrated in my previous comment.. After that case is resolved, search will no longer return symbols that are not visible, and the code will never reach the call to checkAccess.

We could pull this PR as is, since the call to accessCheck is preventing the incorrect code from compiling, but it is not a proper solution. We need to fix search so that it doesn't return symbols that aren't visible. Maybe I can move some of the accessCheck code to search, but I'm not sure if that's the right fix. I'll have to think about it.

SSoulaimane · 2019-04-22T03:05:04Z

I'm not sure you can remove that specific checkAccess() call. when calling search() on the package imports it should find the module b313 since it is a child module and symbolIsVisible() doesn't decide whether you can access a module.

rainers · 2019-04-22T06:44:34Z

I hit a related problem just yesterday: https://issues.dlang.org/show_bug.cgi?id=19816

This reverts commit c9585e0.

SSoulaimane · 2019-04-24T17:42:05Z

src/dmd/access.d

-        }
-    }
-    if (!result && (!(sc.flags & SCOPE.onlysafeaccess) || sc.func.setUnsafe()))
+


This is what you need for the buildkite fails.

+ if (smember.toParent().isTemplateInstance()) + { + return false; // for backward compatibility + }

Thank you. I'd still like to do the refactoring as a followup after this PR is merged. This PR is already quite large, and I'd like to keep the friction low.

JinShil · 2019-04-25T10:22:27Z

Yeah!!! All Green!!! What a relief!!!

This PR is as good as I know how to make it at this time. As @Geod24 mentioned there are 2 error messages that are imperfect. They are not incorrect, but they could be better, which is like many error messages in D today. I did my best to improve them, but it appears they will require some refactoring of DMD to get the information needed. I don't know how to do that yet, and I think it would just add more difficulty to the review of this PR.

I will follow up this PR with @SSoulaimane's suggestion to refactor access.d. This PR is already rather large and I would prefer not to cloud things with a refactoring.

From my point of view, this is good to go.

If you do choose to merge this, please be sure to close #7241.

Thanks @MartinNowak for the initial work, and thanks everyone else for your help along the way.

wilzbach

Looks very good! How about not removing the now failing tests, but just updating their error message with AUTO_UPDATE?

JinShil · 2019-04-25T10:33:31Z

I don't believe I removed any tests, but the diff doesn't show what really happened. Some tests were just consolidated into one (e.g. the checkImports tests). Others were moved (e.g. test15925.d), but the diff shows them as a deletion and an addition, rather than a rename. Please identify a specific test if I'm missing something.

Edit: Also the reason the checkImports tests were consolidated is because this PR removes the -transition=checkimports flags. After doing that, the different tests we all just testing the same thing. So it made sense to only have one.

JinShil · 2019-04-25T11:04:23Z

BTW, this is no longer a WIP

wilzbach · 2019-04-25T12:12:59Z

Edit: Also the reason the checkImports tests were consolidated is because this PR removes the -transition=checkimports flags. After doing that, the different tests we all just testing the same thing. So it made sense to only have one.

All good. Thanks! Sorry for not seeing this immediately.

wilzbach · 2019-04-25T13:11:16Z

BTW another nice thing about this PR is that we finally can convert the existing module imports to selective ones Phobos. In other words, we can all these selective comments:

https://github.com/dlang/phobos/blob/eb38e8e6307f2b3c27b86d2d25ddeac1b400beb5/std/algorithm/iteration.d#L74

RazvanN7 · 2019-04-27T12:22:37Z

Thanks for this awesome work!

JinShil · 2019-04-27T12:32:51Z

And thanks to all of you for you reviews and ferrying this through the process. It feels so good to have this done. 👍

JinShil · 2019-04-27T12:33:46Z

@SSoulaimane If you want to submit a PR to refactor access.d, please do. It was your work, so it would be nice if it were signed with your handle.

SSoulaimane · 2019-04-27T17:59:29Z

@JinShil no it's yours just do it. I'll assist you if I can.

FeepingCreature · 2019-06-28T06:10:44Z

This, specifically 8e9b2ca , seems to have accidentally fixed https://issues.dlang.org/show_bug.cgi?id=19536 . You can now pass a private symbol to a template alias and call it from the template without running into errors.

I'm very happy about this, but is it intentional?

JinShil · 2019-06-28T06:29:18Z

is it intentional?

No, that was not intentional, but I don't see why something like that should be prevented. There is currently an open question about whether we should support public aliases to private symbols. The compiler's behavior is currently inconsistent (some work -- e.g. templates -- and some don't). I don't have a way to force design decisions from the powers that be except through the DIP process, and the DIP process is quite disproportionate when all you want is a thoughtful decision. I think @Geod24 was going to follow up on this topic, but I'm not sure if that happened or will happen.

The current implementation proactively prevents public aliases to private symbols, so a PR to allow it should only involve deleting code. Just by that alone, it seems like we're trying to enforce an arbitrary limitation, and I think it might be best to just to allow it, but that's not really my call to make. I could propose it by submitting a PR, but I'm not sure if I'm prepared to fight the battle that may ensue.

This issue seems to have actually been fixed in: c2eefb0d8d850715f492197b0b1a033d9ad5411a dlang/dmd#9636 Add very basic test facility for independent test files.

b17712_c13.d was apparently fixed in: c2eefb0d8d850715f492197b0b1a033d9ad5411a dlang/dmd#9636 Add very basic test facility for independent test files.

b17712.d is not fixed in dmd, Phobos workaround: 2f20661 dlang#6659 b17712_c13.d was apparently fixed in dmd: c2eefb0d8d850715f492197b0b1a033d9ad5411a dlang/dmd#9636 Add very basic test facility for independent test files.

CyberShadow · 2023-06-14T11:48:43Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=23991

thewilsonator added the Review:WIP Work In Progress - not ready for review or pulling label Apr 17, 2019

JinShil requested review from Geod24, MartinNowak, WalterBright and rainers as code owners April 18, 2019 01:39

Geod24 reviewed Apr 18, 2019

View reviewed changes

test/compilable/checkimports1.d Outdated Show resolved Hide resolved

Geod24 reviewed Apr 18, 2019

View reviewed changes

wilzbach reviewed Apr 19, 2019

View reviewed changes

src/dmd/cli.d Show resolved Hide resolved

wilzbach mentioned this pull request Apr 19, 2019

Add private imports explicitly dlang/dlang-bot#229

Merged

wilzbach mentioned this pull request Apr 19, 2019

Add private imports explicitly vibe-d/vibe.d#2304

Merged

JinShil added 2 commits April 24, 2019 16:50

Remove no longer needed code from access.d

9caf4ec

Revert "Remove no longer needed code from access.d"

f5cfb68

This reverts commit c9585e0.

SSoulaimane reviewed Apr 24, 2019

View reviewed changes

JinShil mentioned this pull request Apr 25, 2019

Add selective imports to rest-js example in support of upcoming DMD PR vibe-d/vibe.d#2305

Merged

wilzbach mentioned this pull request Apr 25, 2019

Remove visiblity and lookup deprecation #7241

Closed

wilzbach approved these changes Apr 25, 2019

View reviewed changes

jacob-carlborg removed the Review:WIP Work In Progress - not ready for review or pulling label Apr 25, 2019

wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 25, 2019

RazvanN7 approved these changes Apr 27, 2019

View reviewed changes

RazvanN7 merged commit d511353 into dlang:master Apr 27, 2019

JinShil deleted the dep_visible branch April 27, 2019 12:32

JinShil mentioned this pull request Apr 27, 2019

access.d: remove obsolete access checks in favor of visibility checks #9721

Merged

MoonlightSentinel mentioned this pull request Feb 13, 2020

Remove unused parameters from access.d #10784

Merged

ntrel mentioned this pull request Jun 20, 2020

Fix Issue #17712 - Add test cases #11300

Closed

ntrel added a commit to ntrel/phobos that referenced this pull request Jun 20, 2020

Fix Issue #17712 - Add test cases

7ec3127

This issue seems to have actually been fixed in: c2eefb0d8d850715f492197b0b1a033d9ad5411a dlang/dmd#9636 Add very basic test facility for independent test files.

ntrel added a commit to ntrel/phobos that referenced this pull request Jun 20, 2020

Issue #17712 - Add test cases

ddb2d87

b17712_c13.d was apparently fixed in: c2eefb0d8d850715f492197b0b1a033d9ad5411a dlang/dmd#9636 Add very basic test facility for independent test files.

ntrel mentioned this pull request Jun 21, 2020

Issue 17712 - Add tests & Revert std.conv.toChars workaround for dmd link error dlang/phobos#7536

Closed

Uh oh!

Comments

Conversation

JinShil commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Apr 17, 2019

Bugzilla references

Testing this PR locally

Uh oh!

thewilsonator commented Apr 17, 2019

Uh oh!

atilaneves commented Apr 17, 2019

Uh oh!

wilzbach commented Apr 17, 2019

Uh oh!

jacob-carlborg commented Apr 17, 2019

Uh oh!

thewilsonator commented Apr 17, 2019

Uh oh!

JinShil commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Geod24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Geod24 Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

JinShil Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Geod24 Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

JinShil Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

JinShil Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wilzbach commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Apr 19, 2019

Uh oh!

rainers commented Apr 19, 2019

Uh oh!

atilaneves commented Apr 19, 2019

Uh oh!

wilzbach commented Apr 19, 2019

Uh oh!

rainers commented Apr 19, 2019

Uh oh!

rainers commented Apr 19, 2019

Uh oh!

JinShil commented Apr 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SSoulaimane commented Apr 22, 2019

Uh oh!

JinShil commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SSoulaimane commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

JinShil commented Apr 17, 2019 •

edited

Loading

JinShil commented Apr 17, 2019 •

edited

Loading

JinShil Apr 18, 2019 •

edited

Loading

wilzbach commented Apr 19, 2019 •

edited

Loading

JinShil commented Apr 20, 2019 •

edited

Loading

JinShil commented Apr 22, 2019 •

edited

Loading

SSoulaimane commented Apr 22, 2019 •

edited

Loading

SSoulaimane Apr 24, 2019 •

edited

Loading

JinShil commented Apr 25, 2019 •

edited

Loading