Moving the discussion from the PR (https://github.com/dlang/dmd/pull/6290#issuecomment-264706485) for better exposure.

Recently I tried to get a bit more involved with preparing and maintaining releases but I see that there are a lot of responses that imply that at least some of my methodologies are either misunderstood or disapproved. So probably better to discuss it to see if we can get on a common ground.

There are several fundamental goals I'd like to pursue:

1) At any given point of time the master branch should be as close to release quality as possible
2) Lack of implementation quality in merged PRs should not become burden of maintainers
3) Deprecation system has to be used for any breaking changes to the point it can be relied on

(1) and (3) are supposed to make life of commercial users easier directly, (2) aims for the same goal by reducing maintenance burden and thus improving release fixup latency. Those all are areas that are currently lacking a lot in D development process and that is rather painful.

Keeping this goal list in mind, I want to propose some consequent practical applications:

a)

    If any breaking change is detected in both master and stable that doesn't
    come at least with detailed explanation in the changelog, it has to be
    reverted unconditionally.

This is a very simple and harmless way to improve on goals (1) and (2). I have noticed that reverting of merged code is often viewed as something damaging and even disrespectful - it isn't! By reverting a PR one immediately fixes the problem in relevant branch causing less trouble for other developers while providing more valuable information (and regression test case) to revive original PR. Compared to trying to fix the problem in place it also puts much less stress on developer as one can take all the time needed.

Similar reasoning applies to fixing regressions in point releases - it is not worth spending time on that unless fix is immediately obvious and trivial. World won't burn if those fixes will get delayed a bit, but providing public release with minimal amount of issues known is urgent.

b)

   Any breaking change that causes compile-time error has to undergo
   deprecation stage. It applies to bug fixes too. If breaking change changes
   code semantics but keeps compiling, it has to provide `-transition=XXX`
   diagnostics to help with migration AND changelog entry explaining what to
   look for.

This is a long standing source of complains from me but I had no idea that purpose of `-transition` flags is not widely obvious. Martin Nowak has drawn this nice table that explains it better:

before change  |  after change | transition process
===================================================
valid          | invalid       | deprecate-then-error
invalid        | valid         | none
invalid        | invalid       | none
valid          | valid         | -transition=feature to find semantic changes

One key thing about this is that adding deprecation is in most cases trivial and adds no extra effort if done right from the beginning. Tracking changes and fixing it later, on the contrary, is a killer for combined productivity.

On 12/4/2016 3:52 PM, Dicebot wrote:
> before change  |  after change | transition process
> ===================================================
> valid          | invalid       | deprecate-then-error
> invalid        | valid         | none
> invalid        | invalid       | none
> valid          | valid         | -transition=feature to find semantic changes
>
> One key thing about this is that adding deprecation is in most cases trivial and
> adds no extra effort if done right from the beginning.

The table makes sense and looks right. But there's a problem with the return scope changes. It's just too much to just wholesale deprecate things all at once. People need time to ease into it, not be faced with a pile of diverse deprecation messages.

Having -transition=safe makes that practical, and also ensures time to correct any unrecognized issues with it.

On 12/4/2016 3:52 PM, Dicebot wrote:
>     If any breaking change is detected in both master and stable that doesn't
>     come at least with detailed explanation in the changelog, it has to be
>     reverted unconditionally.
>
> This is a very simple and harmless way to improve on goals (1) and (2). I have
> noticed that reverting of merged code is often viewed as something damaging and
> even disrespectful - it isn't! By reverting a PR one immediately fixes the
> problem in relevant branch causing less trouble for other developers while
> providing more valuable information (and regression test case) to revive
> original PR. Compared to trying to fix the problem in place it also puts much
> less stress on developer as one can take all the time needed.

This is a rather blunt hammer and I don't see how it is harmless. Reverting it in stable or for a point release, ok. But in master? It may just require a tweak or two. Consider that the PR may fix a severe problem, yet introduce a minor regression. Having an inviolate rule removes judgement from the process.

On 12/05/2016 03:10 AM, Walter Bright wrote:
> The table makes sense and looks right. But there's a problem with the return scope changes. It's just too much to just wholesale deprecate things all at once.

It will need to happen at some point anyway. Without having those deprecation implemented we can't even know how much existing code is affected.

> People need time to ease into it, not be faced with
> a pile of diverse deprecation messages.

We can introduce `-preview=XXX` flag for that purpose but the key thing is that it still has to use deprecations and not errors for all scope related changes. Otherwise you introduce dangerous disparity between what gets previewed and what will eventually become the default.

> Having -transition=safe makes that practical, and also ensures time to correct any unrecognized issues with it.

1) `-transition` flag is not for this purpose. You want this behavior - please introduce new flag group, for example `-preview=scope`. Desire is legit but you have to use a different flag for such purpose.

2) You must not use the very same flag for bug fixes as an excuse to not bother of deprecations. https://github.com/dlang/dmd/pull/6290 is the last one remaining now there was no justification to merge it in first place.

On 12/05/2016 03:15 AM, Walter Bright wrote:
> This is a rather blunt hammer and I don't see how it is harmless.

It is harlmess by a simple criteria that I challenge you to give an example of harm it causes. Reverting a regression fix is only case that I can imagine reverting is worse than fixing.

> Reverting it in stable or for a point release, ok. But in master? It may just require a tweak or two. Consider that the PR may fix a severe problem, yet introduce a minor regression.

THERE IS NO SUCH THING AS MINOR REGRESSSION
THERE IS NO SUCH THING AS MINOR REGRESSSION
THERE IS NO SUCH THING AS MINOR REGRESSSION
THERE IS NO SUCH THING AS MINOR REGRESSSION
THERE IS NO SUCH THING AS MINOR REGRESSSION

> Having an inviolate rule
> removes judgement from the process.

On 12/07/2016 03:56 AM, Dicebot wrote:
> On 12/05/2016 03:15 AM, Walter Bright wrote:
>> This is a rather blunt hammer and I don't see how it is harmless.
> 
> It is harlmess by a simple criteria that I challenge you to give an example of harm it causes. Reverting a regression fix is only case that I can imagine reverting is worse than fixing.
> 
>> Reverting it in stable or for a point release, ok. But in master? It may just require a tweak or two. Consider that the PR may fix a severe problem, yet introduce a minor regression.
> 
> THERE IS NO SUCH THING AS MINOR REGRESSSION
> THERE IS NO SUCH THING AS MINOR REGRESSSION
> THERE IS NO SUCH THING AS MINOR REGRESSSION
> THERE IS NO SUCH THING AS MINOR REGRESSSION
> THERE IS NO SUCH THING AS MINOR REGRESSSION

On a more serious note - not breaking things is much more important than fixing things. Even if existing issue is severe, the fact that it exists for a long time means users have a way to mitigate it. You can't make things worse by delaying it for one release. One the other hand, any issue that causes existing projects to break has sever consequence for ecosystem usability.

I am perfectly fine with "any rule must have exceptions approach". But there has to be at least some basic rule. Productivity of D development suffers from decision reluctance horribly exactly because there is no way to say if something non-trivial is OK to merge unless you come and say so.

https://github.com/dlang/dmd/pull/6290 is perfect example of how absurd it gets. There was a simple task of reverting bunch of prematurely merged pull requests so that their implementation can be improved. Half a days job at most. And yet one month later we are still arguing and discussing and considering and doing nothing. How does it even surprise you after that scope PR doesn't get enough attention?

On 12/6/2016 5:54 PM, Dicebot wrote:
> On 12/05/2016 03:10 AM, Walter Bright wrote:
>> The table makes sense and looks right. But there's a problem with the
>> return scope changes. It's just too much to just wholesale deprecate
>> things all at once.
>
> It will need to happen at some point anyway. Without having those
> deprecation implemented we can't even know how much existing code is
> affected.

That's the point of the flag. It can be done on the user's timescale, not ours.


>> People need time to ease into it, not be faced with
>> a pile of diverse deprecation messages.
>
> We can introduce `-preview=XXX` flag for that purpose but the key thing
> is that it still has to use deprecations and not errors for all scope
> related changes. Otherwise you introduce dangerous disparity between
> what gets previewed and what will eventually become the default.

I agree, but it doesn't have to be deprecations until the flag becomes the default.


>> Having -transition=safe makes that practical, and also ensures time to
>> correct any unrecognized issues with it.
>
> 1) `-transition` flag is not for this purpose. You want this behavior -
> please introduce new flag group, for example `-preview=scope`. Desire is
> legit but you have to use a different flag for such purpose.

It originally was just `-safe`, but I got a lot of flak for that and pressure to use `-transition`. I don't really care what the flag is. It just needs a flag.

I doubt users will really care either if it is -safe, -dip1000, -transition=safe, or -preview=safe.


> 2) You must not use the very same flag for bug fixes as an excuse to not
> bother of deprecations. https://github.com/dlang/dmd/pull/6290 is the
> last one remaining now there was no justification to merge it in first
> place.

There isn't a hard line between what is a bug and what is not. It'd be nice if there was. Pretty much all of these fall under fixing unsafe code.

On 12/6/2016 5:56 PM, Dicebot wrote:
> It is harlmess by a simple criteria that I challenge you to give an
> example of harm it causes.

Let's say there's a bug report that XXX generates corrupt code (such as not initializing a variable). This may work most of the time, or appear to work, but exhibit an error only rarely. But when it does fail, it could cause much disruption to the user's business.

Suppose that a fix is created, but comes with it a regression that some unusual (but correct) code is now broken.

We have the benefit of fixing a silent potentially disastrous problem, while introducing a problem that may be a nuisance, but will at least be obvious to the user.

In other words, if the bug fixed is costlier than the bug introduced, then that is an example of net harm in reverting.

Almost all changes/fixes bring with them at least *some* difference that *somebody* may depend on, however remote the change and however we might roll our eyes that somebody managed to depend on it. :-)

(For example, somebody might be doing hashes on the object file generated.)

On 12/6/2016 6:03 PM, Dicebot wrote:
> On a more serious note - not breaking things is much more important than
> fixing things. Even if existing issue is severe, the fact that it exists
> for a long time means users have a way to mitigate it. You can't make
> things worse by delaying it for one release. One the other hand, any
> issue that causes existing projects to break has sever consequence for
> ecosystem usability.

On the other hand, we cannot progress the language by freezing it. After all, one transitions to newer versions of the language for good reason. Furthermore, bugs (especially safety problems) can be a silent liability for any code that has them.

So, yes, delaying these sorts of fixes can most definitely make things worse for users.

This is why I push hard on these safety fixes. I want them fixed before they suddenly cost somebody on code they thought was fine.


> I am perfectly fine with "any rule must have exceptions approach". But
> there has to be at least some basic rule. Productivity of D development
> suffers from decision reluctance horribly exactly because there is no
> way to say if something non-trivial is OK to merge unless you come and
> say so.

I've approved a lot of PRs that turned out to cause regressions. I don't know of any way to prove that PRs do not introduce regressions. At least we have a test suite that gets continuously added to - that helps enormously.


> https://github.com/dlang/dmd/pull/6290 is perfect example of how absurd
> it gets. There was a simple task of reverting bunch of prematurely
> merged pull requests so that their implementation can be improved. Half
> a days job at most. And yet one month later we are still arguing and
> discussing and considering and doing nothing. How does it even surprise
> you after that scope PR doesn't get enough attention?

What surprises me is the reluctance to put it behind a switch.

>type test7.d
struct A
{
    ~this(){}
}

@safe struct B
{
    A a;
}

>..\dmd test7 -transition=safe
test7.d(6): Error: @safe destructor 'test7.B.~this' cannot call @system destructor 'test7.A.~this'

>..\dmd test7
test7.d(6): Error: @safe destructor 'test7.B.~this' cannot call @system destructor 'test7.A.~this'

This is with my version of the code, which includes all of the scope related changes. One of the difficulties I have with all these branches is what the state of the code is in each. Arbitrary reversions make this situation even worse, making it much harder for me to figure out what the right fix is.

BTW, one of the difficulties is the change that stopped -transition=safe from setting -dip25. All of the scope changes were built on top of the dip25 changes - zero testing was done on setting those flags independently.

There are no reported issues with any of the other cases that PR dealt with, so I don't feel that throwing the whole thing out is the best way forward. The best way is to file bugzilla issues and take a look at if just a tweak would suffice.
-----------------------

The tangle of branches on this has reached a point where I suspect the best way forward is to simply abandon them, and I can submit a new PR that is the difference between master and what I have.

On 12/6/2016 5:56 PM, Dicebot wrote:
> THERE IS NO SUCH THING AS MINOR REGRESSSION

BTW, my uncle was a research chemist for a pharmaceutical company in Switzerland. He told me long ago that there was no such thing as a drug without harmful side effects. All drugs have a "therapeutic index", which is the ratio of the good they do vs the harm. The researcher's job was to make that index as high as he could.

It's the same with any real world product. Nothing could be built if, for example, safety is a top priority that trumps all else. Of course, companies and governments always say that safety is their top priority, but that's because they're forced to by the media who will excoriate them for saying anything else.

We do need processes and rules, but they should serve as guides for good judgement, not as substitutes for good judgement.

And that's where we need your help and judgement - in deciding what that "therapeutic index" is for the PRs on a case by case basis.