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 -de switch from Phobos builds to allow deprecations. #9052

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

LightBender
Copy link
Contributor

Remove the -de switch to allow future deprecations in Phobos.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @LightBender! 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

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 run digger -- build "master + phobos#9052"

@dlang-bot dlang-bot merged commit 59b3715 into dlang:master Sep 20, 2024
10 checks passed
@LightBender LightBender deleted the remove-de branch September 20, 2024 13:12
@Geod24
Copy link
Member

Geod24 commented Sep 21, 2024

Why is this necessary ? My understanding is that we want Phobos to build without deprecation in itself, but that doesn't prevent it from exposing deprecated symbols ?

@jmdavis
Copy link
Member

jmdavis commented Sep 21, 2024

Why is this necessary ? My understanding is that we want Phobos to build without deprecation in itself, but that doesn't prevent it from exposing deprecated symbols ?

Two reasons:

  1. -de is fundamentally broken and really should be removed from the compiler entirely, because D metaprogramming routinely tests for whether a piece of code compiles or not, so having a flag which changes what compiles and what doesn't can change which templates get instantiated and which static if branches get compiled in, which will result in compilation errors in some cases but will simply silently change the behavior of code in others. -w has the same problem. Neither flag should exist.

  2. If we compile Phobos with -de, then it becomes impossible to compile unit tests for deprecated symbols. So, for instance, if Adam deprecates some of std.digest for security reasons like he's been discussing, then instead of having a deprecated feature that's still tested, we'd have to remove the unit tests when doing the deprecation, resulting in deprecated code that isn't actually tested and which could therefore silently break. It would be nice to be able to do something like put deprecated on a unittest block to let it use deprecated symbols without complaint, but we can't currently do that. So, treating deprecations as errors currently makes it impossible to test deprecated code.

@Geod24
Copy link
Member

Geod24 commented Sep 22, 2024

-de is fundamentally broken and really should be removed from the compiler entirely

Agree-ish, but that really should be its own discussion point.

It would be nice to be able to do something like put deprecated on a unittest block to let it use deprecated symbols without complaint, but we can't currently do that.

We can absolutely do that. I fail to recall a time when it was not possible.

@jmdavis
Copy link
Member

jmdavis commented Sep 22, 2024

-de is fundamentally broken and really should be removed from the compiler entirely

Agree-ish, but that really should be its own discussion point.

Sure, but I would strongly argue that it's bad practice to use -de, and that as such, we should not be using it with Phobos.

It would be nice to be able to do something like put deprecated on a unittest block to let it use deprecated symbols without complaint, but we can't currently do that.

We can absolutely do that. I fail to recall a time when it was not possible.

Well, the last time I tried to deprecate something in Phobos (which was ultimately shot down in that specific case), putting deprecated on the associated unittest blocks didn't work, and I was forced to remove the tests as part of that PR. Testing deprecated unittest now locally outside of Phobos, it looks like it does work. So, I don't know what happened before. I likely screwed it up in some fashion, but I don't know.

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

Successfully merging this pull request may close these issues.

4 participants