Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove visiblity and lookup deprecation - Take 2 #9058

Closed
wants to merge 1 commit into from
Closed

Remove visiblity and lookup deprecation - Take 2 #9058

wants to merge 1 commit into from

Conversation

JinShil
Copy link
Contributor

@JinShil JinShil commented Dec 10, 2018

This is another try at #7241. It will also resolve Issue 19471.

Much credit to @MartinNowak and @wilzbach. Sorry, I didn't know how to rebase the original PR.

There are a couple of outstanding issues with this PR, and it will need help from reviewers:

  1. test/compilable/test15177.d does not pass. It's generating code with __traits(allmembers) and that's bringing in symbols that aren't visible. I don't know what the correct behavior is.
  2. I'm not terribly happy with the kind of error messages being emitted.

My suggestion to reviewers: Look at the test cases and see if the output makes sense.

cc @RazvanN7, @Geod24

@dlang-bot
Copy link
Contributor

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#9058"

@jacob-carlborg
Copy link
Contributor

test/compilable/test15177.d does not pass. It's generating code with __traits(allmembers) and that's bringing in symbols that aren't visible. I don't know what the correct behavior is.

Hmm, the test15117a module is imported and all symbols are public in that module. To me it looks like it should work.

Not sure if this is related but __traits(allMembers) (and possible other __traits) need to be able to read private symbols, we have agreed on that. Otherwise you'll get an error when iterating the result of __traits(allMembers) when it contains private symbols, even tough you're not trying to access them. It's not even possible to check if a symbol is private and skip it.

@JinShil
Copy link
Contributor Author

JinShil commented Dec 10, 2018

I'm really sorry folks. I spent the whole day on this, and the rabbit hole just keeps getting deeper. I can't afford the time to follow it right now; I thought it would be just a few hours. Maybe another time. I'm really sorry. I tried.

@JinShil JinShil closed this Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants