Deprecate use of private variables from other modules by RazvanN7 · Pull Request #6676 · dlang/dmd

RazvanN7 · 2017-04-04T10:57:30Z

Since #6660 (the work of the devil) was reverted, this PR implements the same feature except this time a deprecation message is issued and not an error

mathias-lang-sociomantic

Needs test and release notes entry

mathias-lang-sociomantic · 2017-04-05T13:35:11Z

src/ddmd/dimport.d

                {
+                    // This deprecation should turn into an error as soon as possible
+                    if (importedSymbol.prot().kind == PROTprivate)
+                        mod.deprecation(loc, "member '%s' is private", names[i].toChars());


I'd rephrase as something like "Accessing private member FQN is deprecated"

How does "module $module member $name is not accessible because it is private". I think having the word "deprecated" in the message is redundant since the phrase starts with "Deprecation: ".

Sounds good 👍

RazvanN7 · 2017-04-10T10:48:39Z

@mathias-lang-sociomantic I've never done a release entry. Could you guide me with the process of doing it?

Thanks

mathias-lang-sociomantic · 2017-04-10T10:53:15Z

@RazvanN7 : https://github.com/dlang/dmd/tree/master/changelog

RazvanN7 · 2017-04-11T11:19:45Z

Any ideas on how to solve the continous-integration/jenkins error in vibe.d ? I see that the deprecation message causes some linking errors and I think that vibe.d should be updated to not use/import private members from other modules

PetarKirov · 2017-04-11T22:52:03Z

Any ideas on how to solve the continous-integration/jenkins error in vibe.d ?

No need to worry, these linker errors are not caused by your pull request, see #6682 (comment)

ibuclaw · 2017-05-01T08:26:52Z

Shouldn't the mechanism for bug34 catch this - or am I misremembering this morning and that only affects protection for non-local imports? Also what about protected and package?

RazvanN7 · 2017-05-09T18:36:44Z

@ibuclaw I don't think that protected and package have anything to do with this PR. anyway, how do we move forward from here?

ibuclaw · 2017-05-09T19:02:58Z

If they suffer from the same bug, then they must be fixed also.

ibuclaw

I think the fix should be in access.d, not where you currently have it.
Consider the following example for fail15896:

import imp15896 : publicbar, privatebar, packagebar;
int func()
{
    publicbar += 1;
    privatebar += 1;
    packagebar +=1;
    return 0;
}

This should throw a compile-time error where marked with NG below:

int bar = 4;
public ref int publicbar()       { return bar; }  // OK
private ref int privatebar()     { return bar; }  // NG
package ref int packagebar()     { return bar; }  // NG

Therefore this PR should also ensure the same behaviour for variables:

public int publicbar = 4;   // OK
private int privatebar = 4; // OK (but should be NG)
package int packagebar = 4; // OK (but should be NG)

RazvanN7 · 2017-06-07T11:23:56Z

@ibuclaw That is what this PR does for private members. I think that package should be the subject of a different PR.

ibuclaw · 2017-06-07T15:41:25Z

It's the same bug, so they should be fixed together.

andralex · 2017-06-07T15:49:31Z

@MartinNowak should decide on this. Do we fix all in one deprecation shot, or do we stagger fixes?

andralex · 2017-10-28T13:45:21Z

ping @MartinNowak @WalterBright

andralex · 2017-10-28T13:45:28Z

@RazvanN7 please rebase

ibuclaw · 2018-01-10T10:56:35Z

src/ddmd/dimport.d

+                if (importedSymbol)
                {
+                    // This deprecation should turn into an error as soon as possible
+                    if (importedSymbol.prot().kind == PROTprivate)


I think it would be better if you used one of these functions.

dmd/src/dmd/access.d

Lines 481 to 525 in 8d6efa5

extern (C++) bool symbolIsVisible(Module mod, Dsymbol s)

{

// should sort overloads by ascending protection instead of iterating here

s = mostVisibleOverload(s);

final switch (s.prot().kind)

{

case PROTundefined: return true;

case PROTnone: return false; // no access

case PROTprivate: return s.getAccessModule() == mod;

case PROTpackage: return s.getAccessModule() == mod || hasPackageAccess(mod, s);

case PROTprotected: return s.getAccessModule() == mod;

case PROTpublic, PROTexport: return true;

}

}

/**

* Same as above, but determines the lookup module from symbols `origin`.

*/

extern (C++) bool symbolIsVisible(Dsymbol origin, Dsymbol s)

{

return symbolIsVisible(origin.getAccessModule(), s);

}

/**

* Same as above but also checks for protected symbols visible from scope `sc`.

* Used for qualified name lookup.

*

* Params:

* sc = lookup scope

* s = symbol to check for visibility

* Returns: true if s is visible by origin

*/

extern (C++) bool symbolIsVisible(Scope *sc, Dsymbol s)

{

s = mostVisibleOverload(s);

final switch (s.prot().kind)

{

case PROTundefined: return true;

case PROTnone: return false; // no access

case PROTprivate: return sc._module == s.getAccessModule();

case PROTpackage: return sc._module == s.getAccessModule() || hasPackageAccess(sc._module, s);

case PROTprotected: return hasProtectedAccess(sc, s);

case PROTpublic, PROTexport: return true;

}

}

Probably

if (!symbolIsVisible(sc, importedSymbol) { error; }

dlang-bot · 2018-01-12T09:59: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

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.

RazvanN7 · 2018-01-12T10:00:13Z

@ibuclaw @mathias-lang-sociomantic I rebased and updated the code. Is this cool now?

ibuclaw

Change seems fine.

ibuclaw · 2018-01-12T10:30:49Z

changelog/private.dd

+imported private members were visible from other modules. This
+represents a severe violation of the private keyword specification.
+At this point the use of selectively imported private members has
+been deprecated and should be transformed into an error as soon


Will be transformed?

Then again the wording in general could be improved.

Proposed wordsmithing:

$(LINK2 $(ROOT_DIR)spec/attribute.html#visibility_attributes, The specification states) that a private member is visible only from within the same module. Prior to this release, due to a bug, some imported private members were visible from other modules, violating the specification. Beginning with this release, accessing said private members from outside the module in which they are declared will result in a deprecation message which has been scheduled to be changed to an error in approximately 1 year.

Thanks. I copy-pasted your words.

JinShil · 2018-01-17T09:14:11Z

src/dmd/dsymbolsem.d

                {
+                    import dmd.access : symbolIsVisible;
+                    if (!symbolIsVisible(sc, importedSymbol))
+                         imp.mod.deprecation(imp.loc, "member '%s' is not accessible", imp.names[i].toChars());


Please add the following comments to make it easier to follow up on changing it to an error.

// @@@DEPRECATED_2018-01@@@ // Deprecation for 1 year after which this should be changed to an error

Shouldn't this be 2019?

It was deprecated in 2018-01. It will be changed to an error in 2019-01. I read the comment as the date is was deprecated, not the date it should be changed to an error.

That's not how we use this pattern at Phobos. The string is intended to be greppable in the future, i.e. you can do grep "@@@DEPRECATED_2019" for everything that needs maintenance action in 2019 and grep "@@@DEPRECATED_2019_01" for everything within a specific month.

That doesn't seem right to me, but I'll play along. It probably needs to be disambiguated somehow. Please offer @RazvanN7 a suggestion so we can get this merged.

Well it's the existing practice to document deprecations, see e.g. https://github.com/wilzbach/phobos/blob/complex/std/datetime/timezone.d#L145

How else are you going to find them? I already did -> Just change to 2018 -> 2019

JinShil · 2018-01-17T09:15:41Z

Is there a bug report for this?

RazvanN7 · 2018-01-17T09:34:10Z

@JinShil There was a bug report which closed when I fixed the issue to issue an error. Later when the PR was reverted so that the error could be transformed into a deprecation the bug report was no longer opened. It's this one : https://issues.dlang.org/show_bug.cgi?id=15896. If this PR is merged then we will be ok.

MartinNowak

Please don't merge before I reviewed this.

MartinNowak · 2018-01-29T20:35:46Z

I don't quite follow the action in BlackEdder/ggplotd#45, seems like noone figured out why the compiler runs into an infinite loop.

Clearly this is not allowed

alias T = int;
alias T = T;

and so

import std.stdio : writeln;
alias writeln = writeln;

void main()
{
    writeln("Hello");
}

should print an error about writeln aliasing itself, but it currently compiles.
In particular because it is syntactially very close to how you construct overload sets, this is likely sth. that people will try from time to time.
https://dlang.org/hijack.html#overload-sets

Unfortunately there is no explanation for the infinite loop introduced with this PR.
Just fixing code that triggers the bug to pass CI, seems like the wrong action here.

I can reproduce an ICE for the following similar case.

import std.stdio : writeln;
import bar;
alias writeln = writeln; // <- should error, as the selectively imported writeln already is an alias
alias writeln = bar.writeln; // OK, overload set of selectively imported writeln and bar.writeln

void main()
{
    writeln();
}

This is the profile I get from the inifite loop in the compiler with this PR and testing ggplotd.

59.12%  dmd         dmd                  [.] _D3dmd6access19mostVisibleOverloadFCQBi7dsymbol7DsymbolZQv
10.33%  dmd         dmd                  [.] AliasDeclaration::isOverloadable
 8.67%  dmd         dmd                  [.] AliasDeclaration::toAlias
 5.72%  dmd         dmd                  [.] Prot::isMoreRestrictiveThan
 3.92%  dmd         dmd                  [.] Declaration::prot
 3.40%  dmd         dmd                  [.] Dsymbol::isFuncAliasDeclaration
 1.74%  dmd         dmd                  [.] Dsymbol::isFuncDeclaration
 1.48%  dmd         dmd                  [.] OverDeclaration::isOverDeclaration
 1.48%  dmd         dmd                  [.] Dsymbol::isTemplateDeclaration
 1.03%  dmd         dmd                  [.] AliasDeclaration::isAliasDeclaration
 0.72%  dmd         dmd                  [.] Dsymbol::isOverDeclaration
 0.71%  dmd         dmd                  [.] OverDeclaration::isOverloadable
 0.70%  dmd         dmd                  [.] Dsymbol::toAlias

MartinNowak

Looks OKish, only just found the other half in #7760, please fix the infinite loop though.

MartinNowak · 2018-01-29T20:39:11Z

changelog/private.dd

@@ -0,0 +1,3 @@
+Deprecate the use of private selectively imported members
+
+$(LINK2 $(ROOT_DIR)spec/attribute.html#visibility_attributes, The specification states) that a private member is visible only from within the same module. Prior to this release, due to a bug, some imported private members were visible from other modules, violating the specification. Beginning with this release, accessing said private members from outside the module in which they are declared will result in a deprecation message which has been scheduled to be changed to an error in approximately 1 year.


s/some imported private members/private selective imports/
Also just mention that it has been deprecated, we have a general schedule for deprecations (6 months), but deviate from that when it seems too disturbing.

MartinNowak · 2018-01-29T20:40:44Z

test/fail_compilation/fail15896.d

+/*
+TEST_OUTPUT:
+---
+fail_compilation/fail15896.d(11): Deprecation: module `imports.imp15896` member `thebar` is not accessible


s/accessible/visible/
Accessibility is the old protection scheme that will soon be removed.
Also check the other visibility messages, they should be more sth. like X of mod M is not visible from mod N or so.

MartinNowak · 2018-01-29T20:47:40Z

src/dmd/dsymbolsem.d

                AliasDeclaration ad = imp.aliasdecls[i];
                //printf("\tImport %s alias %s = %s, scope = %p\n", toPrettyChars(), aliases[i].toChars(), names[i].toChars(), ad._scope);
-                if (imp.mod.search(imp.loc, imp.names[i]) /*, IgnorePrivateImports*/)
+                Dsymbol importedSymbol = imp.mod.search(imp.loc, imp.names[i] /*, IgnorePrivateImports */);


The comment was added with #7760.

We should at some point change the logic from pulling out a symbol and then checking it's visibility, to passing the visibility level to search and reduce it with according to import protection (like a funnel).
This would also be necessary to fix package import pkg.common;, but requires quite some changes and partial order of visibility.

RazvanN7 · 2018-01-30T09:53:51Z

@MartinNowak The infinte loop introduced by this PR in ggplotd is due to the function mostVisibleOverload in dmd/access.d which I did not touch. The problem here is, as you have stated, the fact that the compiler allows alias ApplyLeft = ApplyLeft. I wanted to move on with this (which is a separate issue) and then investigate the recursive alias problem.

andralex · 2018-02-01T19:34:34Z

@MartinNowak @ibuclaw anything else to do here?

JinShil · 2018-02-02T03:57:59Z

Restarting Jenkins due to dlang/ci#151

wilzbach · 2018-02-02T04:08:36Z

That usually doesn't help. Often you need to give Jenkins a git commit sha to reload the config - though in this case it might work...
(In my experience if the dlang/ci PR has been made at the same day -> needs new git Sha for a real restart)

MartinNowak · 2018-02-02T15:58:25Z

@MartinNowak The infinte loop introduced by this PR in ggplotd is due to the function mostVisibleOverload in dmd/access.d which I did not touch.

But you did call it in a situation where it wasn't previously called, that counts as introducing a bug ;).

@RazvanN7 I think the way forward here is very clear and simple, make

import std.stdio : writeln;
alias writeln = writeln;

an error (maybe even without deprecation as it seems extremely rare in commited code).
Error message should be sth. like recursive alias, use a qualified name to create overload sets.

The reason why should fix this in advance is because overload sets are rather complex, and this looks like a plausible syntax to combine the selectively imported writeln into an overload set named writeln.

I'll try to work on a PR later today, but at latest for the 2.079.0 beta on 2018-02-14, so that we don't release with that bug.

MartinNowak · 2018-02-02T16:34:00Z

That usually doesn't help. Often you need to give Jenkins a git commit sha to reload the config - though in this case it might work...
(In my experience if the dlang/ci PR has been made at the same day -> needs new git Sha for a real restart)

To clarify this, by design, hitting replay on Jenkins checks out the same repo versions that were used last time. This does only apply to those repositories for which we use Jenkin's internal checkout mechanism.
That includes the ci repo with the runPipeline.groovy script and the repo that triggered the build.
So hitting replay won't pick up any changes for those 2 repos, filed as dlang/ci#160.

addressed

wilzbach · 2018-02-02T17:42:54Z

(closed and reopened for Semaphore to know about this PR)

wilzbach · 2018-02-02T17:49:47Z

@MartinNowak applied your comments to the changelog and fixed a few other nits in the changelog message.

timotheecour · 2018-02-21T01:34:05Z

wrote #7930 to fix issue with alias X=X making compiler hang

mathias-lang-sociomantic previously requested changes Apr 5, 2017

View reviewed changes

RazvanN7 force-pushed the Deprecate_Issue_15896 branch from 145c14b to b37d7c1 Compare April 10, 2017 10:45

RazvanN7 force-pushed the Deprecate_Issue_15896 branch from b37d7c1 to 7052cba Compare April 11, 2017 11:06

mathias-lang-sociomantic mentioned this pull request Apr 11, 2017

Changelog entry for Issue 15896 #6686

Closed

andralex approved these changes Jun 5, 2017

View reviewed changes

ibuclaw previously requested changes Jun 6, 2017

View reviewed changes

dlang-bot added Review:Needs Rebase Review:Needs Work labels Jan 1, 2018

ibuclaw reviewed Jan 10, 2018

View reviewed changes

RazvanN7 force-pushed the Deprecate_Issue_15896 branch from 1443839 to 40ca250 Compare January 12, 2018 09:59

dlang-bot removed Review:Needs Work Review:Needs Rebase labels Jan 12, 2018

ibuclaw reviewed Jan 12, 2018

View reviewed changes

JinShil suggested changes Jan 17, 2018

View reviewed changes

RazvanN7 force-pushed the Deprecate_Issue_15896 branch from 60a6d7b to 01d934e Compare January 29, 2018 10:19

Deprecate use of private variables from other modules

7b556cb

RazvanN7 force-pushed the Deprecate_Issue_15896 branch from 01d934e to 7b556cb Compare January 29, 2018 10:43

MartinNowak requested changes Jan 29, 2018

View reviewed changes

MartinNowak approved these changes Jan 29, 2018

View reviewed changes

wilzbach mentioned this pull request Jan 31, 2018

End the deprecation period of using the result of a comma expression dlang/dlang.org#2151

Merged

MartinNowak added the Merge:auto-merge label Feb 2, 2018

consistent error message for selective import of invisible symbol

b662e2a

MartinNowak force-pushed the Deprecate_Issue_15896 branch from 48bee80 to b662e2a Compare February 2, 2018 16:49

dlang-bot removed the Merge:auto-merge label Feb 2, 2018

MartinNowak added the Merge:auto-merge label Feb 2, 2018

wilzbach mentioned this pull request Feb 2, 2018

Use more selective imports at module imports dlang/phobos#5584

Closed

wilzbach closed this Feb 2, 2018

wilzbach reopened this Feb 2, 2018

Improve changelog entry

1b14a23

dlang-bot removed the Merge:auto-merge label Feb 2, 2018

wilzbach added the Merge:auto-merge label Feb 2, 2018

dlang-bot merged commit 15c8da1 into dlang:master Feb 2, 2018

wilzbach mentioned this pull request Feb 21, 2018

fix Issue 18480 - dmd 2.079 hangs #7930

Merged

	extern (C++) bool symbolIsVisible(Module mod, Dsymbol s)
	{
	// should sort overloads by ascending protection instead of iterating here
	s = mostVisibleOverload(s);
	final switch (s.prot().kind)
	{
	case PROTundefined: return true;
	case PROTnone: return false; // no access
	case PROTprivate: return s.getAccessModule() == mod;
	case PROTpackage: return s.getAccessModule() == mod \|\| hasPackageAccess(mod, s);
	case PROTprotected: return s.getAccessModule() == mod;
	case PROTpublic, PROTexport: return true;
	}
	}

	/**
	* Same as above, but determines the lookup module from symbols `origin`.
	*/
	extern (C++) bool symbolIsVisible(Dsymbol origin, Dsymbol s)
	{
	return symbolIsVisible(origin.getAccessModule(), s);
	}

	/**
	* Same as above but also checks for protected symbols visible from scope `sc`.
	* Used for qualified name lookup.
	*
	* Params:
	* sc = lookup scope
	* s = symbol to check for visibility
	* Returns: true if s is visible by origin
	*/
	extern (C++) bool symbolIsVisible(Scope *sc, Dsymbol s)
	{
	s = mostVisibleOverload(s);
	final switch (s.prot().kind)
	{
	case PROTundefined: return true;
	case PROTnone: return false; // no access
	case PROTprivate: return sc._module == s.getAccessModule();
	case PROTpackage: return sc._module == s.getAccessModule() \|\| hasPackageAccess(sc._module, s);
	case PROTprotected: return hasProtectedAccess(sc, s);
	case PROTpublic, PROTexport: return true;
	}
	}

		@@ -0,0 +1,3 @@
		Deprecate the use of private selectively imported members

		$(LINK2 $(ROOT_DIR)spec/attribute.html#visibility_attributes, The specification states) that a private member is visible only from within the same module. Prior to this release, due to a bug, some imported private members were visible from other modules, violating the specification. Beginning with this release, accessing said private members from outside the module in which they are declared will result in a deprecation message which has been scheduled to be changed to an error in approximately 1 year.

Uh oh!

Comments

Conversation

RazvanN7 commented Apr 4, 2017

Uh oh!

mathias-lang-sociomantic left a comment

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 commented Apr 10, 2017

Uh oh!

mathias-lang-sociomantic commented Apr 10, 2017

Uh oh!

RazvanN7 commented Apr 11, 2017

Uh oh!

PetarKirov commented Apr 11, 2017

Uh oh!

ibuclaw commented May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RazvanN7 commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw commented May 9, 2017

Uh oh!

ibuclaw left a comment • 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!

RazvanN7 commented Jun 7, 2017

Uh oh!

ibuclaw commented Jun 7, 2017

Uh oh!

andralex commented Jun 7, 2017

Uh oh!

andralex commented Oct 28, 2017

Uh oh!

andralex commented Oct 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw Jan 10, 2018 • 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!

dlang-bot commented Jan 12, 2018

Bugzilla references

Uh oh!

RazvanN7 commented Jan 12, 2018

Uh oh!

ibuclaw left a comment

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!

JinShil Jan 17, 2018 • 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!

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!

JinShil Jan 17, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented May 1, 2017 •

edited

Loading

RazvanN7 commented May 9, 2017 •

edited

Loading

ibuclaw left a comment •

edited

Loading

ibuclaw Jan 10, 2018 •

edited

Loading

JinShil Jan 17, 2018 •

edited

Loading

JinShil Jan 17, 2018 •

edited

Loading

MartinNowak commented Jan 29, 2018 •

edited

Loading

MartinNowak left a comment •

edited

Loading

RazvanN7 commented Jan 30, 2018 •

edited

Loading