compile druntime with -preview=dip1021

[WalterBright]

https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1021.md

This adds some important checks:

Given the following:

@safe
void foo()
{
     int* p;
     {
        int x;
        p = &x;
     }
}

The compiler gives:

test.d(8): Error: address of variable x assigned to p with longer lifetime

when the -preview=dip1021 switch is used. C++ is moving in this direction (Nick Treveaven).

[dlang-bot]

Thanks for your pull request, @WalterBright!

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

[WalterBright]

Failing due to:

src/core/demangle.d(755): Error: more than one mutable reference to `this` in arguments to `core.demangle.reencodeMangled.PrependHooks.parseType()`

I'll look into that.

[WalterBright]

I'm baffled by:

src/core/time.d-mixin-1354(1354): Error: more than one mutable reference to `su` in arguments to `core.time.Duration.split!("seconds", "nsecs").split!(long, long).split()`
src/core/sync/config.d(64): Error: template instance `core.time.Duration.split!("seconds", "nsecs")` error instantiating

I don't know where the multiple mutable ref is coming from. @jmdavis can you have a look at this, please?

[ibuclaw]

Does dip1021 affect signatures?

[WalterBright]

@ibuclaw no it does not affect signatures. It only adds additional checks.

[WalterBright]

@jmdavis I took a break and came back later, and figured it out. On to the next problem!

[WalterBright]

Next problem:

src/object.d(394): Error: `@safe` function `object.__unittest_L359_C7` cannot call `@system` function `object.opEquals!(F, G).opEquals`
src/object.d(275):        `object.opEquals!(F, G).opEquals` is declared here

I'll look at that tomorrow.

[WalterBright]

Next problem(s):

src\object.d(4768): Error: more than one mutable reference of `c1` in arguments `c1` and `c1` to `object.__cmp!(C, C).__cmp()`
src\object.d(4768): Error: more than one mutable reference of `c1` in arguments `c1` and `c1` to `object.__cmp!(C, C).__cmp()`
src\core\lifetime.d(149): Error: `@safe` function `core.lifetime.__unittest_L138_C7.__lambda1` cannot call `@system` function `core.lifetime.emplace!(SafeClass, int).emplace`
src\core\lifetime.d(1573):        which calls `core.lifetime.emplace!(SafeClass, int).emplace.fwd!(__param_1).fwd`
src\core\lifetime.d(1923):        which calls `core.lifetime.move!int.move`
src\core\lifetime.d(1977):        which calls `core.lifetime.moveImpl!int.moveImpl`
src\core\lifetime.d(97):        `core.lifetime.emplace!(SafeClass, int).emplace` is declared here

[WalterBright]

A problem showing up repeatedly is __cmp() should have const parameters, not mutable ones.

[jmdavis]

A problem showing up repeatedly is __cmp() should have const parameters, not mutable ones.

It's templated so that it can work with both const and mutable objects, and it needs to give that opCmp isn't necessarily const.

[WalterBright]

Another mysterious failure with no message:

make[2]: Leaving directory 'D:/a/dmd/dmd/druntime/test/exceptions'
make[1]: Leaving directory 'D:/a/dmd/dmd/druntime'
make: *** [Makefile:446: unittest-debug] Error 2
make: Leaving directory 'D:/a/dmd/dmd/druntime'
Error: Process completed with exit code 2.

[WalterBright]

@jmdavis the parameters must be const, not just templated to take either. Dip1021 looks to see if two mutable arguments are passed to __cmp(), and fails it if it does, because __cmp() accepts mutable arguments and configures itself to have mutable parameters.

[jmdavis]

@jmdavis the parameters must be const, not just templated to take either. Dip1021 looks to see if two mutable arguments are passed to __cmp(), and fails it if it does, because __cmp() accepts mutable arguments and configures itself to have mutable parameters.

But opCmp is not necessarily const, and it can't necessarily be const depending on how the classes are implemented. If we try to require it, it's going to result in implementations having to cast away const.

In general, requiring any particular attribute causes problems even if a particular attribute makes sense in most cases. I don't know what needs to happen here with what you're trying to do with DIP 1021, but it's going to be a problem if opCmp requires const, and if it doesn't require const, then __cmp can't require it.

[WalterBright]

__cmp can cast away const in its call to opCmp.

(Even if not using the borrow checker, the rule of only one mutable reference to the same object is good style.)

[jmdavis]

__cmp can cast away const in its call to opCmp.

(Even if not using the borrow checker, the rule of only one mutable reference to the same object is good style.)

Okay, but then what if opCmp mutates? If a class has a mutable opCmp, it's allowed to mutate its state, so wouldn't it then be violating the type system because of what __cmp is doing? It's already problematic that we have that hack in the free function, opEquals (which is there because of backwards compatibility but really shouldn't be there). And just because opEquals and opCmp can't mutate their state in terms of the comparison logic, that doesn't mean that they can't mutate other state that isn't part of the comparison logic (e.g. a mutex).

And sure, it may be good in general to not be passing multiple mutable references to the same object to a function, but that's going to be inevitable at least some of the time when you start dealing with stuff like generic code or with references to the same object where you don't know that they're reference to the same object. And it's expected that stuff like the comparison operators work when the same object is on both sides of the comparison.

[WalterBright]

If a class has a mutable opCmp, it's allowed to mutate its state, so wouldn't it then be violating the type system because of what __cmp is doing?

Yes. And it's the only way forward. opCmp was put in long before const appeared. Having comparison mutate state is just as broken if a cast is not required to match the function prototype.

[jmdavis]

If a class has a mutable opCmp, it's allowed to mutate its state, so wouldn't it then be violating the type system because of what __cmp is doing?

Yes. And it's the only way forward. opCmp was put in long before const appeared. Having comparison mutate state is just as broken if a cast is not required to match the function prototype.

We cannot be requiring const any more than we can be requiring pure or @nogc. D's const is too restrictive to require, and plenty of types will not work with it. Functions like __cmp and __opEquals are templated in part because the sets of attributes that work with a particular user-defined type can vary considerably, and in the general case, we cannot require any attributes on a user-defined type, or there will be code that will not work - code that should be perfectly valid. This is particularly true for common operations such as == or <.

It's quite reasonable for a function like opCmp to not be able to be const, because it's doing something with its state that is logically const but not fully const (e.g. doing something with a mutex). IMHO, if this DIP is making it so that we can't allow that, then it needs to be revisited.

And in the general case, IMHO, if const is being used on a function template parameter, that's a code smell. If the list of types being used is restricted to types where const will work with the functions being used, then it's fine (e.g. if you've templated a function on the element type of an array and restricted it to character types, because that function is supposed to operate on any array of characters - since then you know that const will work with those types), but if you're dealing with user-defined types, then it's a big problem to require const, just like it's a big problem to require pretty much any attribute, because not all types will work with that. We should be writing code that works with a variety of attributes, not requiring any particular attributes. They're simply too restrictive, and it consistently causes problems when we do.

So, if this DIP is going to require that const be put on the parameters to templated functions just because they might end up with multiple references to the same data being passed in, then IMHO, that's a big problem. There may very well be cases where we want to prevent that, but in the general case, it's something that needs to work, because const is too restrictive to work in the general case even if it happens to work in many cases.

[tgehr]

If a class has a mutable opCmp, it's allowed to mutate its state, so wouldn't it then be violating the type system because of what __cmp is doing?

Yes. And it's the only way forward. opCmp was put in long before const appeared. Having comparison mutate state is just as broken if a cast is not required to match the function prototype.

There is a difference between state and memory representation of said state. For example, consider data structures with amortized runtime guarantees. They necessarily have to restructure the internal representation even for queries, even though the logically represented data does not change.

[atilaneves]

If a class has a mutable opCmp, it's allowed to mutate its state, so wouldn't it then be violating the type system because of what __cmp is doing?

Yes. And it's the only way forward. opCmp was put in long before const appeared. Having comparison mutate state is just as broken if a cast is not required to match the function prototype.

Should the usual comparison function mutate state? No. Should we disallow functions that for some reason need to? I don't think so. Should the answer to those rare cases that need it be "it's ok, go break the type system"? I don't think so.

[WalterBright]

@atilaneves the problem is that Object.opCmp takes mutable parameters, and a const class object cannot be passed to it. The same problem exists for Object.toString(), Object.toHash(), and Object.opEquals(). @andralex and I have talked about this for years, and have been unable to come up with a solution that doesn't disrupt everything.

But I finally found one!

With an edition, have the compiler internally rewrite those declarations (and their overrides) to be const, but don't change the name mangling. A hack, but it ought to work.

[jmdavis]

@atilaneves the problem is that Object.opCmp takes mutable parameters, and a const class object cannot be passed to it. The same problem exists for Object.toString(), Object.toHash(), and Object.opEquals(). @andralex and I have talked about this for years, and have been unable to come up with a solution that doesn't disrupt everything.

But I finally found one!

With an edition, have the compiler internally rewrite those declarations (and their overrides) to be const, but don't change the name mangling. A hack, but it ought to work.

The core problem is that we cannot require that these functions work as const (or having any other attribute), because that's going to restrict what can be done with them. Really, those functions shouldn't be on Object at all. Derived classes should be defining them based on what works for their particular hierarchies, and as long as the relevant druntime code is templated instead of being written to work on Object, it's a non-issue (though the fact that that work hasn't been completed means that it's continued to be a problem).

The partial solution that's currently in-place is that the free function opEquals and __cmp are templated. That way, if your derived class declares those functions with other attributes, it will work regardless of what Object has so long as you're using references that aren't Object. And if the druntime hooks can be properly templated, then we don't have the need for Object to be used in any of that. We could then even look at outright removing all of those functions from Object (though backwards compatibility may not allow that similar to issues with removing the monitor). Objects being compared or hashed or whatever just causes problems.

Regardless, we cannot be requiring const or any other attributes on core stuff like this, or it means that folks are going to have to violate the type system for basic operations to work on some types. Every single attribute is too restrictive to require it for all types - const included. We need to be better support those attributes for types where they do work, but we cannot require them. In general, that means using templates for core stuff so that the code will work with any set of attributes, and for classes, it means getting to the point that we simply don't call functions on Object. We're partway there, and we need to get the rest of the way there, not start adding attributes in places that will require folks to violate the type system to avoid them.

[WalterBright]

@jmdavis thank you for your reasoned analysis. It is off topic for the PR, so I request reposting your comment in the n.g. I already opened a thread for it. Thanks!

[ljmf00]

I have a few errors on druntime side, I found here at Weka by trying out --preview=dip1021. I'll try to report them soon. CC @JohanEngelen