www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - What to do when you want to add attributes to your class functions?

reply RazvanN <razvan.nitu1305 gmail.com> writes:
Let's say you have a library class that exposes some methods. At 
the beginning you did not focus on the safety, nogcness etc. of 
your methods, you just wanted to have something working. You 
publish your class, people start using it and now you are in a 
phase where you would like your methods to be callable from 
attributed contexts. So you go on and make your code safe, 
nothrow, nogc, pure. However, now you are faced with a large 
breaking change because suddenly you force your users to respect 
these attributes. This seems like a severe limitation to me. 
Morever, look at this code:

```d
class Test
{
     void fun()  safe {}
}

class TestDerived : Test
{
     override void fun()
     {
         int a;
         int *b = &a;
     }
}
```

You get: `(11): Error: cannot take address of local `a` in 
` safe` function `fun``. The override is not even marked as  safe 
and the compiler does not indicate that the function is  safe 
because it overrides a safe function. Anyway, I digress.

The point is, there is no way for a library owner to satisfy both 
people that want attributes and people that don't. If you do not 
put attributes on your code from the beginning, users of your 
class will not be able to call the super method from an 
attributed scope. If you put attributes on your methods, there is 
no way to bypass them in your overrides. I understand that if we 
would allow to relax the restrictions in overrides that would 
lead to the ability of calling unsafe code from safe contexts, 
but the current design makes it cumbersome to use classes with 
attributes. It just seems simpler to never use attributes at all 
(with classes).

Now, the entire point of this post is to ask what should we do in 
the case of: https://github.com/dlang/dmd/pull/14432. The story 
is: the methods of core.sync.condition can be made  nogc; the 
only reason why we did not do that is because we would throw some 
SyncErrors (which are allocated with the gc); the fix is trivial, 
we just use an abort function that does not throw anything, it 
just ends execution. However, now, if we mark the methods as 
 nogc we break downstream code (phobos as well) because suddenly 
those methods need to abide to new restrictions.  How do we get 
out of this stale mate?

Cheers,
RazvanN
Sep 16 2022
next sibling parent reply Kagamin <spam here.lot> writes:
On Friday, 16 September 2022 at 09:25:58 UTC, RazvanN wrote:

 Now, the entire point of this post is to ask what should we do 
 in the case of: https://github.com/dlang/dmd/pull/14432. The 
 story is: the methods of core.sync.condition can be made  nogc; 
 the only reason why we did not do that is because we would 
 throw some SyncErrors (which are allocated with the gc)


Are those really `Error`s? Then throw one statically allocated 
instance with generic message.
Sep 16 2022
parent reply RazvanN <razvan.nitu1305 gmail.com> writes:
On Friday, 16 September 2022 at 11:33:13 UTC, Kagamin wrote:

 On Friday, 16 September 2022 at 09:25:58 UTC, RazvanN wrote:

 Now, the entire point of this post is to ask what should we do 
 in the case of: https://github.com/dlang/dmd/pull/14432. The 
 story is: the methods of core.sync.condition can be made 
  nogc; the only reason why we did not do that is because we 
 would throw some SyncErrors (which are allocated with the gc)


 Are those really `Error`s? Then throw one statically allocated 
 instance with generic message.


That is not the problem. That has been taken care of. The issue 
is that users of Condition now have to abide by the rules of nogc.
Sep 16 2022
parent reply Kagamin <spam here.lot> writes:
Overridable functions there forward to templated functions:
```
void notify()
{
	notify!(typeof(this))(true);
}
void notify() shared
{
	notify!(typeof(this))(true);
}
```
So you make the templated function  nogc and add a  nogc method:
```
void notify()
{
	notify!(typeof(this))(true);
}
void notify() shared
{
	notify!(typeof(this))(true);
}
void notify2() shared  nogc
{
	notify!(typeof(this))(true);
}
```
Sep 16 2022
parent Kagamin <spam here.lot> writes:
To reduce template bloat declare implementation nontemplated:
```
void notify()
{
	shared me=cast(shared)this;
	me.notifyImpl();
}
void notify() shared
{
	notifyImpl();
}
void notify2() shared  nogc
{
	notifyImpl();
}
private void notifyImpl() shared  nogc
{
	code...
}
```
Sep 16 2022
prev sibling next sibling parent reply Adam D Ruppe <destructionator gmail.com> writes:
On Friday, 16 September 2022 at 09:25:58 UTC, RazvanN wrote:

 However, now you are faced with a large breaking change because 
 suddenly you force your users to respect these attributes. This 
 seems like a severe limitation to me.


It is *absolutely* necessary to maintain the integrity of 
interfaces.

see 
http://dpldocs.info/this-week-in-d/Blog.Posted_2022_07_11.ht
l#run-time-dispatch for some tangentially related discussion


 The override is not even marked as  safe and the compiler does 
 not indicate that the function is  safe because it overrides a 
 safe function.


The error message could perhaps point it out, but this behavior 
is also quite useful, letting implementations usually just work.



 The point is, there is no way for a library owner to satisfy 
 both people that want attributes and people that don't.


You could have two branches of the interface. It is always 
allowed to add restrictions to child classes, so you could have

class Base { void foo(); }
class StrictBase : Base { void foo()  nogc pure etc; }

then people use those. Or, you can have

class Base {
    int foo_strict()  nogc pure etc {}
    final void foo() { }
}

Though that gets weird if you wanted to override the other one i 
still use final on purpose here cuz that's less weird than having 
both and needing to choose one.


 It just seems simpler to never use attributes at all (with 
 classes).


Or never use the attributes at all with anything. They don't 
actually provide much value anyway.


 However, now, if we mark the methods as  nogc we break 
 downstream code (phobos as well) because suddenly those methods 
 need to abide to new restrictions.


In this case, either the overload or an outright breaking change 
is the right thing to do. I find it very unlikely these are 
actually overridden often. But the possible breaking of error 
handling needs to be considered regardless of attributes.
Sep 16 2022
parent reply RazvanN <razvan.nitu1305 gmail.com> writes:
On Friday, 16 September 2022 at 12:42:40 UTC, Adam D Ruppe wrote:

 On Friday, 16 September 2022 at 09:25:58 UTC, RazvanN wrote:

 However, now you are faced with a large breaking change 
 because suddenly you force your users to respect these 
 attributes. This seems like a severe limitation to me.


 It is *absolutely* necessary to maintain the integrity of 
 interfaces.


I completely understand why that is the case, it's just that it 
makes it
difficult to use both features together.


 see 
 http://dpldocs.info/this-week-in-d/Blog.Posted_2022_07_11.ht
l#run-time-dispatch for some tangentially related discussion


 The override is not even marked as  safe and the compiler does 
 not indicate that the function is  safe because it overrides a 
 safe function.


 The error message could perhaps point it out, but this behavior 
 is also quite useful, letting implementations usually just work.



 The point is, there is no way for a library owner to satisfy 
 both people that want attributes and people that don't.


 You could have two branches of the interface. It is always 
 allowed to add restrictions to child classes, so you could have

 class Base { void foo(); }
 class StrictBase : Base { void foo()  nogc pure etc; }

 then people use those. Or, you can have

 class Base {
    int foo_strict()  nogc pure etc {}
    final void foo() { }
 }


This works under the assumption that you have already created your
code with a thought to attributes. However, there is a ton of code
that is not attributed but could be.


 Though that gets weird if you wanted to override the other one 
 i still use final on purpose here cuz that's less weird than 
 having both and needing to choose one.


 It just seems simpler to never use attributes at all (with 
 classes).


 Or never use the attributes at all with anything. They don't 
 actually provide much value anyway.


Reasonable people could disagree.


 However, now, if we mark the methods as  nogc we break 
 downstream code (phobos as well) because suddenly those 
 methods need to abide to new restrictions.


 In this case, either the overload or an outright breaking 
 change is the right thing to do. I find it very unlikely these 
 are actually overridden often. But the possible breaking of 
 error handling needs to be considered regardless of attributes.


The overload does not seem like a good solution to me. Right now 
I am making
the functions nogc, but what if in the future we could make them 
 safe (it's
just for illustrative purposes, I don't know if any of those 
functions could
be made  safe)? We create yet another overload?
Sep 16 2022
parent Adam D Ruppe <destructionator gmail.com> writes:
On Friday, 16 September 2022 at 13:02:44 UTC, RazvanN wrote:

 what if in the future we could make them  safe
 We create yet another overload?


Yes. You'll probably want to think it through to try to get it 
right the first time (and not add new attributes to the language 
without good reason).

You are publishing an interface with certain restrictions. You 
need to decide what those restrictions are or be willing to make 
breaking changes.

In this case, I really do think the breaking change is the right 
thing to do. Quite unlikely the addition of ` nogc` here will 
*actually* break anything - a search of the dub code might be 
able to give stronger evidence for this too. But changing an 
exception to a different mechanism, including a different 
allocation, might break things in practice so i really expect 
that's where you should be focusing...
Sep 16 2022
prev sibling parent reply Dukc <ajieskola gmail.com> writes:
On Friday, 16 September 2022 at 09:25:58 UTC, RazvanN wrote:

 Let's say you have a library class that exposes some methods. 
 At the beginning you did not focus on the safety, nogcness etc. 
 of your methods, you just wanted to have something working. You 
 publish your class, people start using it and now you are in a 
 phase where you would like your methods to be callable from 
 attributed contexts. So you go on and make your code safe, 
 nothrow, nogc, pure. However, now you are faced with a large 
 breaking change because suddenly you force your users to 
 respect these attributes.


As with anything, it's about finding the most practical solution 
overall for everyone involved. Unless you're publishing low-level 
system code (not very likely with classes), the usual 
cost-benefit radio from best to worst is IMO ` safe`, `pure`, 
`nothrow`, ` nogc`.

Considering a library done from stratch, I'd say that most 
abstract functions should have first two of those attributes but 
not the last two. But if you need to consider backwards 
compatibility, one option to consider is going only with ` safe`. 
Less breakage than with ` safe pure`, and breaking code can 
cowboy it with ` trusted // GREENWASHED: Not reviewed for safety` 
to each of those. Or better yet, actually review those functions 
for safety.


 This seems like a severe limitation to me.


It is a limitation, that's for sure. But I think it may be less 
severe than you think. Missing `nothrow` and ` nogc` attributes 
are far from the worst problems in a typical application code. 
Many would say that there they aren't even worth the visual 
clutter they are.

And if you don't mark your code `pure` and ` safe` well before 
you publish it for everyone to use in non-alpha state, it 
suggests that you don't consider even those attributes quite 
essential. It's arguable if that's a good attitude, but it's 
quite popular judging by this forum.



 It just seems simpler to never use attributes at all (with 
 classes).


That's close to a good advice with regards to `nothrow` and 
especially ` nogc`. I think that even if your abstract function 
should in principle be implemented without throwing or garbage 
collecting, it's often a good idea to omit those attributes just 
to make life easier for the child function implementor. Maybe 
even omit `pure`. But at least use ` safe` unless you have strong 
specific reasons not to (like heavy issues with backwards 
compatibility  - I'd probably defer adding ` safe` to next major 
version of the library). Making an abstract function ` system` 
puts much more burden to anyone who uses the class and cares 
about safety, than it saves burden from those implementing the 
children functions.


 Now, the entire point of this post is to ask what should we do 
 in the case of: https://github.com/dlang/dmd/pull/14432.


Disclaimer: I had only a surface look on what `Condition` seems 
to do. It's pretty likely I'm not on map on how it works and how 
it's supposed to be used.

I would not add ` nogc` to the interface of this function. A 
reasonable implementation might use the heap to do some 
bookkeeping and might want to allocate on `notify`. An ideal 
implementation would not do so, but abstract classes should 
generally be easy to implement even from not-so-perfect code.

It can be questioned why the same class acts both as an abstract 
class for other implementations, and an implementation in itself. 
If there was a separate `final class` that implemented 
`Condition`, it's members could be ` nogc`. The downside would be 
that the class instance would be a bit bigger since D object grow 
with number of interfaces/classes they inherit - maybe that's why.
Sep 16 2022
parent "H. S. Teoh" <hsteoh qfbox.info> writes:
On Fri, Sep 16, 2022 at 10:55:46PM +0000, Dukc via Digitalmars-d wrote:

 On Friday, 16 September 2022 at 09:25:58 UTC, RazvanN wrote:

 Let's say you have a library class that exposes some methods. At the
 beginning you did not focus on the safety, nogcness etc. of your
 methods, you just wanted to have something working. You publish your
 class, people start using it and now you are in a phase where you
 would like your methods to be callable from attributed contexts. So
 you go on and make your code safe, nothrow, nogc, pure. However, now
 you are faced with a large breaking change because suddenly you
 force your users to respect these attributes.




One way this could have worked is, if your library code was written with
the most restrictive attributes that would still allow it to work.  Then
code with less restrictives attributes would still be able to call your
code (because your code would be covariant with the more relaxed
attributes). E.g., if your code was written to be `pure` as much as
possible, then it could be called from both pure and impure code.
However, if you didn't pay attention to purity in the first place and a
lot of your code is impure, then pure code would not be able to call
your code.  Similarly, if your code was written to be  safe (whether or
not you initially write the attribute explictly), then  system code
could still call it.

However, this approach may run into problems when user code may inherit
from your class.  Derived class overrides cannot expand the
permissiveness of the overridden base class method's attributes; it can
only restrict it -- e.g., if a base class method is pure, then it would
be invalid to allow a derived class to override it with an impure
method, because then pure code could invoke the impure code via the base
class's pure interface. This implies that base class methods must be
maximally permissive, in order to accomodate derived classes. But that
means that it would not be callable from a more restrictive context.
Thus this defeats our goal.

//

The other way to do it is to use templates and allow the compiler to do
attribute inference. You'd write your code under maximal restrictions,
and use unittests to enforce purity, nothrow, etc., but leave the actual
template code unattributed to allow the compiler to infer attributes. So
if the user code is pure, your code would not introduce impurity and it
would work. If the user code is impure, the compiler just infers impure
and it would still work.

However, template methods cannot be overridden by inheritance, so we
haven't really solved the inheritance issue either.


[...]

 Now, the entire point of this post is to ask what should we do in the
 case of: https://github.com/dlang/dmd/pull/14432.




[...]

Good question. :/  Given that in this case the base class API is already
fixed, seems the best you could do is to add attributed overloads in the
derived class. Which is an ugly solution. :-(


T

-- 
I think Debian's doing something wrong, `apt-get install pesticide', doesn't
seem to remove the bugs on my system! -- Mike Dresser
Sep 16 2022