Skip to content

fix Issue 16085 - wrong visibility warning for overloaded alias symbol#5847

Merged
WalterBright merged 1 commit intodlang:stablefrom
MartinNowak:fix16085
Aug 9, 2016
Merged

fix Issue 16085 - wrong visibility warning for overloaded alias symbol#5847
WalterBright merged 1 commit intodlang:stablefrom
MartinNowak:fix16085

Conversation

@MartinNowak
Copy link
Member

@MartinNowak MartinNowak commented Jun 7, 2016

  • mark OverDeclaration as overloadable
  • mark AliasDeclaration as overloadable (depends on aliasee)
  • replace overloadApply with a custom iteration b/c aliases
    must not be resolved for visibility checks (i.e. public aliases to
    private symbols are public)
  • deal with the messy overloading of aliasees (see comments)

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
16085 wrong visibility warning for overloaded alias symbol

src/access.d Outdated
// alias name = sym;
// private void name(int) {}
else if (auto ad = s.isAliasDeclaration())
next = ad.overnext;
Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't seem to work that way, seem like the function is overloadInserted into the aliasee instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seriously, who came up with the stupid term overnext for nextOverload? It makes thinking about this already messy implementation even harder.

- mark OverDeclaration as overloadable
- mark AliasDeclaration as overloadable (depends on aliasee)
- replace overloadApply with a custom iteration b/c aliases
  must not be resolved for visibility checks (i.e. public aliases to
  private symbols are public)
- deal with the messy overloading of aliasees (see comments)
@MartinNowak MartinNowak added the Severity:Regression PRs that fix regressions label Aug 7, 2016
@MartinNowak MartinNowak changed the title [WIP] fix Issue 16085 - wrong visibility warning for overloaded alias symbol fix Issue 16085 - wrong visibility warning for overloaded alias symbol Aug 7, 2016
@MartinNowak MartinNowak force-pushed the fix16085 branch 2 times, most recently from 77ca7e1 to 22dc481 Compare August 7, 2016 17:05
@WalterBright
Copy link
Member

I'm a little concerned about this. Consider:

struct S {
  public int foo(int i);
  private int foo(uint u);
}

Overloading is done before access checking, but this change seems to do it after. Overloading is not supposed to be affected by public/private.

@MartinNowak
Copy link
Member Author

MartinNowak commented Aug 8, 2016

Overloading is done before access checking, but this change seems to do it after. Overloading is not supposed to be affected by public/private.

No, this does not affect overloading! Please compare w/ DIP22

The least protected symbol determines the visibility for overloads.
After overload resolution an additional access check will be performed. Thereby overload resolution remains independent of look-up origin.

We've discussed this before (at least I mentioned this behavior several times to you) and it was part of the DIP22 PR.
#5472 (comment)

@MartinNowak
Copy link
Member Author

Properly determining the visibility of overloads w/ mixed protection is the point of this PR.

* but doesn't recurse nor resolve aliases because protection/visibility is an
* attribute of the alias not the aliasee.
*/
private Dsymbol mostVisibleOverload(Dsymbol s)
Copy link
Member Author

Choose a reason for hiding this comment

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

As the name and comment say, this is just used to determine the combined visibility of overloads. It does not perform any overload resolution.

Copy link
Member Author

Choose a reason for hiding this comment

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

I plan some refactoring to merge the new visibility mechanism with the old one (limiting visibility of private imports). When this is done, the signature would become bool hasVisibleOverload(Dsymbol s, Scope* sc, Prot visibility), but right now we need to dig out an actual symbol to check.

@WalterBright WalterBright merged commit 8238ad7 into dlang:stable Aug 9, 2016
@MartinNowak MartinNowak deleted the fix16085 branch August 9, 2016 10:59
@MartinNowak
Copy link
Member Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Severity:Regression PRs that fix regressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants