-
-
Notifications
You must be signed in to change notification settings - Fork 610
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
Fix Issue 18623 - Documented unittest should not allow private symbol access #13783
Conversation
|
Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#13783" |
src/dmd/expressionsem.d
Outdated
| symbolIsVisible(sc._module, decl) && | ||
| decl.visibility.kind != Visibility.Kind.public_) | ||
| { | ||
| exp.deprecation("`%s` `%s` is `%s` but is used in a public documented unittest", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent exp with 4 spaces instead of 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
This could use a spec PR, changelog entry, and end date for the deprecation. But before that, it should get a yay or nay from @WalterBright and/or @atilaneves . |
|
Access to private symbols is just one of the problems that can manifest in examples generated from unittests. E.g. missing imports is another big issue that is currently detected using the |
|
IMO this is a symptom of the inflexibility of the documentation generator. Normally, if you want your tests to have access only to public symbols, you can enforce this by putting them in a separate module. However, documented The solution is to lift this limitation of the documentation generator and allow documented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strong no from me, for the following reasons:
-
Implementation
This only considerspublicsymbols: What aboutprotected, which is public under certain conditions ? Andpackage? -
Concept
If someone wants to document private symbols, why prevent it ? In the example, the documented unittest is attached to a private symbol, isn't it ? This is the kind of check that belongs to a linter, not the compiler.
|
Thanks @Geod24 for the assessment. I am certain that there are some cases where a private symbol is used in a documented unittest that is not attached to the actual declaration. In those situations, this sort of diagnostic would be useful, however, I agree that a third party tool should be the judge of that (such as our test extractor that use in phobos). |
Missing imports is something a user of a library can fix themselves. Accidental use of a private symbol is not necessarily fixable by that user. |
If you want to support that unusual case, require |
I doubt that many projects will set up a CI test just for this. Instead private symbol use will have to be manually detected and reported. |
Just as a discussion starter.