Print deprecation messages except if -revert=dip1000 is used #12578

atilaneves · 2021-05-24T16:08:04Z

I think this doesn't cover all of them given that I only had to change one test. There are a lot of checks for DIP1000 in the codebase but that don't immediately print out error messages.

I figured it was best to do this part first and investigate more later.

dlang-bot · 2021-05-24T16:08:07Z

Thanks for your pull request and interest in making D better, @atilaneves! 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 + dmd#12578"

Geod24 · 2021-05-25T00:37:02Z

src/dmd/escape.d

    void unsafeAssign(VarDeclaration v, const char* desc)
    {
-        if (global.params.useDIP1000 == FeatureState.enabled && sc.func.setUnsafe())
+        if (sc.func.setUnsafe())


Doesn't this (and the change at L1045) change the semantic of programs not using DIP1000 as well ?
setUnsafe has side effect. We had a bug related to it: https://github.com/dlang/dmd/pull/12127/files#diff-1a1c16e5ec319550bcd4ecb5d3f4ca6ca7ffad78af91f077603617117bb7d0c7R1037-R1038

Ah, I hadn't considered the short-circuiting nature of the check. Thanks!

After checking the code, I think this is fine, since it only has a side-effect when the compiler is determining safety anyway.

Some projects on buildkite are currently failing because formerly @safe functions are now inferred @system, probably because of this setUnsafe()

Geod24 · 2021-05-25T00:39:45Z

I'm not sure I understand the purpose of that PR ? Turn DIP1000 on by default, but with errors as deprecations ?
Because if the aim is to just force people to use -preview=dip1000, a simple line in mars.d would do, but if the aim is to do something similar to what was done with DIP25 (which I hope it is), then perhaps "Turn DIP1000 on by default" would be a more accurate description.
However, what about the (multiple) bugs still present with DIP1000 ?

atilaneves · 2021-05-26T12:49:07Z

if the aim is to do something similar to what was done with DIP25

The aim is indeed to to do what was done for DIP25. I don't think "turn on DIP1000" is accurate - the goal is first to annoy users with deprecation messages and force them to use -revert to make them go away. Once that's in place we can actually turn it on by default.

However, what about the (multiple) bugs still present with DIP1000 ?

We need to fix all of them, post-haste
The most that can happen with this PR is users get deprecation warnings that don't make sense
-revert solves that

Geod24 · 2021-05-27T07:30:46Z

The aim is indeed to to do what was done for DIP25.

👍 👍

We need to fix all of them, post-haste

The point of -preview design's was to lower the barrier of entry of code in the compiler so a design could be ironed out and tried in the wild. If we start to make DIP1000 the default before the design has actually been ironed out, we're both failing at -preview and accepting code we wouldn't have otherwise.

The most that can happen with this PR is users get deprecation warnings that don't make sense

IOW, something completely unacceptable.

-revert solves that

-revert is a way to selectively disable a class of valid deprecations that might prove invasive for some user. Not an excuse to enable an incomplete preview switch.

atilaneves · 2021-05-27T15:15:10Z

Am I missing something wrt DIP1000 design decisions? AFAIK we "only" have to fix bugs.

RazvanN7 · 2021-05-28T10:23:52Z

DIP1000 does suffer from some bugs, however, every feature does. What would be the point when we can safely say "now is the time to enable dip1000 by default"? Are there any glaring holes that need to be fix first? If not, I think that we can go ahead and merge this PR.

dkorpel · 2021-05-28T11:11:29Z

What would be the point when we can safely say "now is the time to enable dip1000 by default"?
Are there any glaring holes that need to be fix first?

Yes, issue 20150 is particularly egregious.

Geod24 · 2021-05-28T14:49:32Z

What would be the point when we can safely say "now is the time to enable dip1000 by default"?

When DIP1000 allow to achieve its stated goal: @safe reference counting.

atilaneves · 2021-06-07T15:01:57Z

What would be the point when we can safely say "now is the time to enable dip1000 by default"?
Are there any glaring holes that need to be fix first?

Yes, issue 20150 is particularly egregious.

I agree this is a blocker. It's one thing for DIP1000 to not catch all memory safety errors due to a bug, it's another entirely to start compiling code that it didn't before that isn't memory safe.

atilaneves · 2021-11-08T18:43:14Z

The blocking issue has been fixed, so I see no reason to not merge this.

dkorpel · 2021-11-09T01:39:15Z

There's still this: https://issues.dlang.org/show_bug.cgi?id=22221

Geod24 · 2021-11-09T02:22:27Z

Also commit message / PR title needs an update

atilaneves · 2021-11-09T13:15:14Z

Also commit message / PR title needs an update

Do they?

atilaneves · 2021-11-09T13:16:32Z

There's still this: https://issues.dlang.org/show_bug.cgi?id=22221

That's... not ideal. But I wouldn't consider it a blocker. Definitely needs to be fixed, though.

Geod24 · 2021-11-09T13:48:41Z

Do they?

Ah, reading again, I see what you mean. I thought it was the original idea of "Print a deprecation message if the user isn't using DIP1000", not "Start deprecating constructs that would error with -preview=dip1000". So I guess not.

dkorpel · 2021-11-09T14:31:53Z

But I wouldn't consider it a blocker. Definitely needs to be fixed, though.

The dmd fix is easy (#12989), the hard part is not breaking Phobos (dlang/phobos#8214). It turns out that fixing scope errors in range code is really cumbersome. You can't explicitly mark every parameter scope since it's generic code, so you want to rely on inference. Without pure and nothrow, scope inference is really bad however (issue 20674), and the error messages don't help either: Instead of saying 'scope inference failed because of line X', functions just fail to be inferred @safe, resulting in e.g.:

std/array.d(1817): Error: @safe delegate std.array.split!string.split.__foreachbody5 cannot call @system function std.range.primitives.put!(Appender!(string[]), string).put

If you annotate put or doPut @safe, then it results in failing __traits(compiles, ) or is(typeof()) tests, giving unhelpful errors like:

std/range/primitives.d(430): Error: static assert: "Cannot put a const(char)[] into a void delegate(scope const(char)[])."

So what do I fear happens when this PR is merged? People upgrade to dmd 2.099 and are presented with a flood of inscrutable deprecation errors, lots of them based on false positives because of bad scope inference. They want to fix them, but after asking how to do that in the forums they receive the answer "nobody knows". That's why I want to get DIP1000 in a good, well-documented state before making it the default.

atilaneves · 2022-04-15T14:49:57Z

Woohoo, green!

src/dmd/func.d

src/dmd/escape.d

atilaneves · 2022-04-15T15:51:34Z

I addressed the latest review comments.

src/dmd/frontend.h

dkorpel · 2022-04-15T16:06:20Z

changelog/dip1000_deprecation_warnings.dd

+
+As an incentive to transition to -preview=dip1000 by default, now the
+compiler prints what would have been error messages if the flag had
+been on as deprecation messages.  This new behaviour can be disabled


Only in explicit @safe functions, function with inference may still silently be @safe while having scope violations.

dkorpel · 2022-04-15T16:13:11Z

The bigger problem with the changelog entry is that it leaves users clueless: what is DIP1000? What does this mean for my code? Where can I find documentation?

There's no tutorial, and the spec is not up to date (dlang/dlang.org#3262 for one, but there's more). While I approve the PR for what is does, I would say it's "blocked" by the lack of documentation.

Co-authored-by: Dennis <dkorpel@users.noreply.github.com>

atilaneves · 2022-04-18T08:04:35Z

I agree we need more documentation, but I don't think that this PR is the place to do it - I can't link to something outdated. What do you suggest to make the changelog entry better?

dkorpel · 2022-04-18T09:25:15Z

What do you suggest to make the changelog entry better?

I'm working on writing documentation for DIP1000. What we could do is already merge this, and then improve the changelog in a subsequent PR, since there is still time before the next release. This way you don't have to keep rebasing this PR in the meantime.

ibuclaw · 2022-04-21T13:36:33Z

changelog/dip1000_deprecation_warnings.dd

@@ -0,0 +1,6 @@
+Print warning messages unless -revert=dip1000 is used


I guess the little detail of adding this option was overlooked? :-)

Yes, unfortunately: #14019

Geod24 reviewed May 25, 2021

View reviewed changes

atilaneves added the Review:Needs Work label May 26, 2021

dkorpel mentioned this pull request Jun 3, 2021

Skip over return parameters for return destination #12601

Closed

atilaneves added the Merge:Blocked label Jun 7, 2021

dkorpel added the dip1000 memory safety with scope, ref, return label Jul 21, 2021

dlang-bot added the Review:stalled label Sep 5, 2021

atilaneves force-pushed the dip1000-deprecation-msgs branch from 7c7ce6c to 5670235 Compare November 8, 2021 18:42

dlang-bot removed the Review:Needs Work label Nov 8, 2021

atilaneves removed the Merge:Blocked label Nov 8, 2021

dlang-bot removed the Review:stalled label Nov 8, 2021

dlang-bot added Review:Needs Rebase Review:Needs Work labels Nov 16, 2021

dlang-bot added the Review:stalled label Feb 7, 2022

dkorpel mentioned this pull request Mar 24, 2022

Print warning if not using either preview or revert DIP1000 switches #12159

Closed

atilaneves force-pushed the dip1000-deprecation-msgs branch from fbb4685 to 33341ee Compare April 15, 2022 10:03

atilaneves requested a review from ibuclaw as a code owner April 15, 2022 11:19

dkorpel requested changes Apr 15, 2022

View reviewed changes

src/dmd/func.d Outdated Show resolved Hide resolved

src/dmd/escape.d Outdated Show resolved Hide resolved

src/dmd/escape.d Outdated Show resolved Hide resolved

dkorpel reviewed Apr 15, 2022

View reviewed changes

src/dmd/frontend.h Outdated Show resolved Hide resolved

dkorpel approved these changes Apr 15, 2022

View reviewed changes

dkorpel added the Review:Needs Changelog A changelog entry needs to be added to /changelog label Apr 15, 2022

dkorpel reviewed Apr 15, 2022

View reviewed changes

dkorpel added Merge:Blocked and removed Review:Needs Changelog A changelog entry needs to be added to /changelog labels Apr 15, 2022

atilaneves force-pushed the dip1000-deprecation-msgs branch from 5ec7ffb to 5dd5d5d Compare April 18, 2022 08:03

atilaneves and others added 8 commits April 18, 2022 10:03

Print deprecation messages except if -revert=dip1000

ea490dc

Fix breakage due to state changing of FuncDeclaration.setUnsafe

5a10999

Refactoring

ac6182d

Fix frontend.h

91524bc

isUnsafe -> setUnsafeDIP1000

326e02b

checkUnsafe -> isSafeBypassingInference

8a5d515

Delete FuncDeclaration.checkUnsafe

526eae7

Update src/dmd/frontend.h

dccb5f6

Co-authored-by: Dennis <dkorpel@users.noreply.github.com>

atilaneves force-pushed the dip1000-deprecation-msgs branch from 5dd5d5d to 8020446 Compare April 18, 2022 08:08

Add changelog entry

8020446

atilaneves added Merge:72h no objection -> merge The PR will be merged if there are no objections raised. and removed Merge:Blocked labels Apr 18, 2022

atilaneves merged commit 49d44fa into dlang:master Apr 21, 2022

ibuclaw reviewed Apr 21, 2022

View reviewed changes

dkorpel mentioned this pull request May 6, 2022

Revert dip1000 deprecations for 2.100 #14085

Merged

		@@ -0,0 +1,6 @@
		Print warning messages unless -revert=dip1000 is used

Uh oh!

Print deprecation messages except if -revert=dip1000 is used #12578

Print deprecation messages except if -revert=dip1000 is used #12578

Uh oh!

Conversation

atilaneves commented May 24, 2021

Uh oh!

dlang-bot commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

Geod24 May 25, 2021

Choose a reason for hiding this comment

Uh oh!

atilaneves May 26, 2021

Choose a reason for hiding this comment

Uh oh!

atilaneves Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

dkorpel Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

Geod24 commented May 25, 2021

Uh oh!

atilaneves commented May 26, 2021

Uh oh!

Geod24 commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atilaneves commented May 27, 2021

Uh oh!

RazvanN7 commented May 28, 2021

Uh oh!

dkorpel commented May 28, 2021

Uh oh!

Geod24 commented May 28, 2021

Uh oh!

atilaneves commented Jun 7, 2021

Uh oh!

atilaneves commented Nov 8, 2021

Uh oh!

dkorpel commented Nov 9, 2021

Uh oh!

Geod24 commented Nov 9, 2021

Uh oh!

atilaneves commented Nov 9, 2021

Uh oh!

atilaneves commented Nov 9, 2021

Uh oh!

Geod24 commented Nov 9, 2021

Uh oh!

dkorpel commented Nov 9, 2021

Uh oh!

atilaneves commented Apr 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atilaneves commented Apr 15, 2022

Uh oh!

Uh oh!

dkorpel Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

dkorpel commented Apr 15, 2022

Uh oh!

atilaneves commented Apr 18, 2022

Uh oh!

dkorpel commented Apr 18, 2022

Uh oh!

ibuclaw Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

atilaneves Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

dlang-bot commented May 24, 2021 •

edited

Loading

Geod24 commented May 27, 2021 •

edited

Loading