Fix Issue 21651 - Unimported package doesn't error out when used as p… by BorisCarvajal · Pull Request #12215 · dlang/dmd

BorisCarvajal · 2021-02-21T07:28:43Z

…art of fully qualified type

There are a few instances of phobos relying on this bug.

Geod24 · 2021-02-21T09:21:40Z

Is this the right fix ? To me it looks like a flaw in importAll, which makes symbol visible when they shouldn't.

BorisCarvajal · 2021-02-21T09:57:04Z

From what I know Packages need to be check using checkAccess() as is done in expression.d, IIRC visibility doesn't take into account unless the package happens to be an import symbol, this is another peculiarity of dmd that should be addressed by maybe not inserting import symbols to the current scope.

dlang-bot · 2021-02-22T05:46:38Z

Thanks for your pull request and interest in making D better, @BorisCarvajal! 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 coverage diff by visiting the details link of the codecov check)
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

Auto-close	Bugzilla	Severity	Description
✓	21651	normal	Unimported package doesn't error out when used as part of fully qualified type

Testing this PR locally

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

dub run digger -- build "master + dmd#12215"

RazvanN7 · 2021-02-22T09:31:08Z

src/dmd/todt.d

 import dmd.expression;
 import dmd.func;
 import dmd.globals;
+import dmd.glue;


Is this a result of fixing the bug?

Well, it's to fix a instance of the bug in DMD.

There are two lines a little below:

alias toSymbol = dmd.tocsym.toSymbol; alias toSymbol = dmd.glue.toSymbol;

dmd.glue is not imported but is working because of the bug.

RazvanN7 · 2021-02-22T09:38:38Z

I kind of agree with @Geod24 . symbolIsVisible should definitely be false in this situation. Why would we require a different check?

BorisCarvajal · 2021-02-22T09:50:59Z

I've tracked the necessity of this additional check to #5426

BorisCarvajal · 2021-02-22T10:01:38Z

I agree that maybe checkAccess(Package) should be merged with symbolIsVisible, the former needs an Scope parameter to work, though.

BorisCarvajal · 2021-02-22T11:59:10Z

I should've noted that the compiler already shows the error if the qualified name is encapsulated as an expression (DotIdExp) vs a type.

For example in the test test/fail_compilation/imports/test21651a.d

// both statements should be error
imports.test21651b.T a;
int b = imports.test21651b.x;   <-- currently prints "Error: undefined identifier `ice10598b` in package `imports`, perhaps add `static import imports.ice10598b;`"

// so the bug occurs mainly is the following cases:
imports.test21651b.T a;
alias a = imports.test21651b.T;
SomeTemplate!(imports.test21651b.T);

// anything else is already an error, like:
imports.test21651b.somefunction();
++imports.test21651b.variable;
auto v = imports.test21651b.x + imports.test21651b.x;

@RazvanN7, So there is nothing to deprecate.
And if imports had not been previously public by default I think the instances of code using this bug would be almost zero.

RazvanN7 · 2021-02-22T12:21:31Z

@RazvanN7, So there is nothing to deprecate.

The fact that we had to modify druntime and phobos to accomodate this change and we still have buildkite failing more than 10 projects is more than enough to warrant a deprecation.

BorisCarvajal · 2021-02-24T20:19:12Z

That deprecation already happened 5 years ago -> #5426

CyberShadow · 2021-02-25T04:38:19Z

That deprecation already happened 5 years ago -> #5426

How do I get the deprecation to show up for ae? I don't see it.

Are you sure that the deprecation you have in mind covers all the cases broken by this PR?

Geod24 · 2021-02-25T04:56:03Z

That deprecation already happened 5 years ago -> #5426

It clearly didn't. Phobos and druntime compile with deprecations as errors, if there was a deprecation in place the code wouldn't have compiled and you wouldn't have had to fix them. So please make this a deprecation.

BorisCarvajal · 2021-03-02T23:55:42Z

Sorry, what I meant was that the intent of #5426 was to deprecate using fully-qualified names to bypass imports.
This deprecation succeeded and now it's an error but unfortunately due to parser/semantic differences between types and expressions not all the cases were covered, for example:

import std.algorithm;

enum a = std.range.isForwardRange!string;  // Error.  Deprecation happened 5 years ago.

alias isFR = std.range.isForwardRange;   // Absurdly, this still works.
enum b = isFR!string;

Anyway, it seems adding a deprecation message is the way to go.

BorisCarvajal · 2021-03-03T00:05:37Z

What should be the deprecation period or version to put inside the @@@DEPRECATED@@@ comment?

RazvanN7 · 2021-03-03T08:31:12Z

@BorisCarvajal According to DIP1013 [1], the minimum required deprecation period is 10 non-patch releases. Since we have 6 releases per year [2], I guess that the minimum period is 20 months. I would round it up to 2 years just to have a well rounded number, but that is up to you.

[1] https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1013.md
[2] https://dlang.org/changelog/release-schedule.html

RazvanN7 · 2021-03-04T10:34:31Z

test/compilable/ice10598.d

+compilable/imports/ice10598a.d(5): Deprecation: module imports.ice10598b is not accessible here, perhaps add 'static import imports.ice10598b;'
+compilable/imports/ice10598a.d(5): Deprecation: module imports.ice10598b is not accessible here, perhaps add 'static import imports.ice10598b;'
+---
+*/


maybe move this to fail_compilation and add REQUIRED_ARGS: -de

RazvanN7 · 2021-03-04T10:35:42Z

test/fail_compilation/imports/test21651a.d

@@ -0,0 +1,5 @@
+module imports.test21651a;
+
+// both statements should be error


The contents of this file should be moved to fail_compilation/test21651.d. That way we use only 2 files.

RazvanN7 · 2021-03-04T10:36:53Z

test/fail_compilation/test21651.d

+/* TEST_OUTPUT:
+---
+fail_compilation/imports/test21651a.d(4): Deprecation: module imports.test21651b is not accessible here, perhaps add 'static import imports.test21651b;'
+fail_compilation/imports/test21651a.d(5): Error: undefined identifier `test21651b` in package `imports`, perhaps add `static import imports.test21651b;`


This error is not necessary as it is covered already by other tests. Simply compile with -de and keep the minimum required lines of code to reproduce the code that this PR actually fixes.

RazvanN7 · 2021-03-04T10:39:24Z

src/dmd/typesem.d

+                // @@@DEPRECATED_2.096@@@
+                // Should be an error in 2.106. Just remove the deprecation call
+                // and uncomment the null assignment
+                deprecation(loc, "%s %s is not accessible here, perhaps add 'static import %s;'", sm.kind(), sm.toPrettyChars(), sm.toPrettyChars());


here does not sound very professional. I suggest replacing it with the module name.

Also, is the static really necessary?

This is the exact message of the original deprecation.

Fair enough.

RazvanN7

One last thing and we are good to go. This requires a changelog entry since it is deprecating behavior. I know that this is just a slipped case of a former deprecation, still, folks reading the changelog should be notified about it.

changelog/fqn-bypass-deprecation.dd

…art of fully qualified type

BorisCarvajal mentioned this pull request Feb 21, 2021

import 'std.variant.Algebraic' properly dlang/dub#2108

Merged

BorisCarvajal force-pushed the fix21651 branch from e7ad076 to 3f063d6 Compare February 22, 2021 05:46

dlang-bot added the Severity:Bug Fix label Feb 22, 2021

This was referenced Feb 22, 2021

Remove a few uses of fully qualified names that rely on a DMD bug dlang/druntime#3380

Merged

Remove a few uses of fully qualified names that rely on a DMD bug dlang/phobos#7807

Merged

RazvanN7 reviewed Feb 22, 2021

View reviewed changes

BorisCarvajal force-pushed the fix21651 branch 2 times, most recently from 45f68d3 to 8243f10 Compare March 4, 2021 10:05

RazvanN7 requested changes Mar 4, 2021

View reviewed changes

BorisCarvajal force-pushed the fix21651 branch from 8243f10 to 5bff382 Compare March 4, 2021 11:17

RazvanN7 requested changes Mar 4, 2021

View reviewed changes

BorisCarvajal force-pushed the fix21651 branch 3 times, most recently from dda8bf2 to aed8584 Compare March 4, 2021 15:11

RazvanN7 reviewed Mar 4, 2021

View reviewed changes

changelog/fqn-bypass-deprecation.dd Outdated Show resolved Hide resolved

RazvanN7 reviewed Mar 4, 2021

View reviewed changes

changelog/fqn-bypass-deprecation.dd Outdated Show resolved Hide resolved

RazvanN7 reviewed Mar 4, 2021

View reviewed changes

changelog/fqn-bypass-deprecation.dd Outdated Show resolved Hide resolved

Fix Issue 21651 - Unimported package doesn't error out when used as p…

ddf3d07

…art of fully qualified type

BorisCarvajal force-pushed the fix21651 branch from aed8584 to ddf3d07 Compare March 5, 2021 14:10

RazvanN7 approved these changes Mar 8, 2021

View reviewed changes

RazvanN7 merged commit 7f866e2 into dlang:master Mar 8, 2021

		@@ -0,0 +1,5 @@
		module imports.test21651a;

		// both statements should be error

Uh oh!

Conversation

BorisCarvajal commented Feb 21, 2021

Uh oh!

Geod24 commented Feb 21, 2021

Uh oh!

BorisCarvajal commented Feb 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Feb 22, 2021

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Feb 22, 2021

Uh oh!

BorisCarvajal commented Feb 22, 2021

Uh oh!

BorisCarvajal commented Feb 22, 2021

Uh oh!

BorisCarvajal commented Feb 22, 2021

Uh oh!

RazvanN7 commented Feb 22, 2021

Uh oh!

BorisCarvajal commented Feb 24, 2021

Uh oh!

CyberShadow commented Feb 25, 2021

Uh oh!

Geod24 commented Feb 25, 2021

Uh oh!

BorisCarvajal commented Mar 2, 2021

Uh oh!

BorisCarvajal commented Mar 3, 2021

Uh oh!

RazvanN7 commented Mar 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BorisCarvajal commented Feb 21, 2021 •

edited

Loading