Fix Issue 3720 - Taking address of member functions possible without an instance

[RazvanN7]

struct S {
    int a;
    void fun() {
        this.a = 1;
    }
}

void main() {
    auto fp = &S.fun;
    fp();
}

The type of S.fun is set to void*. That way, you cannot call fp directly.

Next step would be to make delegate.funcptr return a void* also.

I had to modify some dmd tests because now typeof(S.fun) is going to return void*. I expect such cases to be rare, but this will definitely require a changelog entry and a spec PR. An alternative would be to trigger a deprecation when typeof(&S.fun) is encountered. What do you think?

[dlang-bot]

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

Auto-close	Bugzilla	Severity	Description
✓	3720	critical	Taking address of member functions possible without an instance

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

1. Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

2. Change the base branch of your PR to stable

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

[RazvanN7]

This seems to test the same thing as the above delegate test. What am I missing @ibuclaw ?

[Herringway]

Why? What value does this inconsistency bring to the language?

[Geod24]

No. This is very clearly a hack. Changing the typeof so the pointer cannot be called is wrong.
Not to mention, we need more typing on delegate, not less.

[adamdruppe]

Yeah, this is definitely wrong. What I'd like to do is formalize the delegate abi a little so we can actually define it. So it might return void function(S* _this) instead of void function(). Unhide the hidden parameter and define things so that makes sense consistently.

(well since it is a delegate and the type of S is erased in there, it'd probably have to be void* context, but still, actually expose it somehow)

[RazvanN7]

Most people will agree that taking the address of S.fun makes sense only if you want to compare it with another address. Another use case (probably the single one) is to assign it to another delegate's funcptr, but I would argue that that is a niche use case. And all of those would still work if we make the funcptr property have void* type.

Taking the address of the delegate is like going to live into the wild, the type system does not protect you anymore and you need to specify everything yourself (via casts). I don't see how making this ugly pattern legal by typing it more is going to help. We need to discourage people to use this kind of code. Typing it even more is the opposite of simplifying the language.

I understand that this may not be the best solution, but @Geod24 @adamdruppe your comments are not constructive at all. Comments like "it's wrong", "it should be like this" without providing a rationale are not helpful at all. Could you maybe present your use cases so that we reach a consensus? I think that your use cases may have other solutions that do not require making the language more complex.

[Herringway]

Most people will agree that taking the address of S.fun makes sense only if you want to compare it with another address. Another use case (probably the single one) is to assign it to another delegate's funcptr, but I would argue that that is a niche use case. And all of those would still work if we make the funcptr property have void* type.

I disagree. Because maintaining interior pointers is unreliable for structs, storing delegates isn't feasible. That leaves us with function pointers, which are currently unusable due to being underdefined, or hacks like this. This should be more defined, not less.

Furthermore, taking the address of S.fun is only allowed in @system code. Writing @system code is already like going to live in the wild. If you want to be protected by the type system, you use @safe.

I would also like to point out that casting void* to a function pointer is undefined behaviour in C(++), and I'm not aware of D being any different in this regard. If that is the case, then there's nothing of value to be gained from this change.

Taking the address of S.fun should return something useful, or simply be an error.

[adamdruppe]

Most people will agree that taking the address of S.fun makes sense only if you want to compare it with another address.

Where's your evidence for this? Of course, even if you did prove most people would agree, declaring that makes it right is still an appeal to popularity fallacy, so where is your actual rationale for it?

The existence of tests that had to be commented out (one of which btw, you merged - just last year! - to close another bug report) ought to show that there is another use case to this. How do you justify this regression?

My proposal is to match the actual ABI and make the function both type-safe and actually callable with a runtime function pointer without as many unreliable casts. (Note that with a template alias, you can call S.fun now by attaching a child this to it.)

[JohanEngelen]

Most people will agree that taking the address of S.fun makes sense only if you want to compare it with another address. Another use case (probably the single one) is to assign it to another delegate's funcptr, but I would argue that that is a niche use case. And all of those would still work if we make the funcptr property have void* type.

Taking the address of the delegate is like going to live into the wild, the type system does not protect you anymore and you need to specify everything yourself (via casts). I don't see how making this ugly pattern legal by typing it more is going to help. We need to discourage people to use this kind of code. Typing it even more is the opposite of simplifying the language.

I understand that this may not be the best solution, but @Geod24 @adamdruppe your comments are not constructive at all. Comments like "it's wrong", "it should be like this" without providing a rationale are not helpful at all. Could you maybe present your use cases so that we reach a consensus? I think that your use cases may have other solutions that do not require making the language more complex.

I think you should split this into two parts:

1. S.fun currently has no proper meaning. It should be an error. If people use it, then explain that it currently has no meaning in the language. Yes it may have worked for them, but only because of "luck" (remember that D is not assembly, and the value of a pointer just happens to be something you can muck with, but may not have a real meaning in the D language).
2. Start debate about what S.fun could possibly mean, make a DIP, etc.

Point 1 needs to be fixed asap, because it leads to hard to find bugs.
Part of the fix could be to provide a solution path for people that did use it for hacks (comparing function similarity is a hack, assigning it to another delegate's funcptr is also a hack); this could be done by introducing a new druntime function or a trait.

Point 2 is a discussion that should not be done inside this PR.

[RazvanN7]

@JohanEngelen Typing it as void* does offer an alternative for people. You just have to manually type the result of &S.fun to whatever you need it to be. Simply making it an error will actually cause more breakage. Also, I don't see the point of adding new druntime functions for use of this feature because we will then have to deprecate them also when we come up with a new design.

I think that what could be done is add a deprecation for taking the address of &S.fun. Maybe that is the path of least resistance. Or type is as void* and also output a deprecation/warning stating that this is dangerous.

[JohanEngelen]

@JohanEngelen Typing it as void* does offer an alternative for people. You just have to manually type the result of &S.fun to whatever you need it to be. Simply making it an error will actually cause more breakage.

My viewpoint is that this is an "accepts invalid" bug. Hence breakage is desired.

[Geod24]

I understand that this may not be the best solution, but @Geod24 @adamdruppe your comments are not constructive at all. Comments like "it's wrong", "it should be like this" without providing a rationale are not helpful at all. Could you maybe present your use cases so that we reach a consensus? I think that your use cases may have other solutions that do not require making the language more complex.

My comment was solely about the solution you provide, not the bug you are tackling. I can express it in more words, if you'd like:

The compiler should have as much static information as possible / convenient. Because with more static information, we can take better decisions: detect errors earlier, generate more efficient code / diagnostic, etc...
This PR, as I understand it, removes some static information (typed function => void*). A byproduct of this is that the compiler is not able to use this function (it doesn't even know it's a function). But this conflict with our goal to have as much static information as we can. So it's wrong because the fix is not at the right place. Preventing taking the address of it, on the other hand, doesn't have the same downside.

[RazvanN7]

@Geod24 I have implemented typing &S.fun as void function(S*). I am putting the context pointer at the end of the parameter list as that is where the compiler puts it, however, there is one problematic case: variadic functions. In case of a variadic function, the context pointer is currently put before ..., however, I am not sure where the compiler puts it in normal circumstances.

[RazvanN7]

With this change, I think that delegate.funcptr becomes useless as it was mainly used to be assigned the address of some member function. Since we are effectively changing the type of the address of a member function we are breaking all the code that uses funcptr that way.

[ibuclaw]

I am putting the context pointer at the end of the parameter list as that is where the compiler puts it,

Wait what? Context pointer is the first parameter.

[RazvanN7]

@ibuclaw That's how I implemented it at first, but then I noticed that I'm getting segfaults for functions that have more than 0 parameters.

It was surprising for me also.

[RazvanN7]

Initially, I put the context pointer in the type as being the first parameter, but then this example issued a segfault:

  struct S
  { 
      int a;
      void fun(int _)
      {
          this.a = 1;
      }   
  }
  
  void main()
  {
      auto fp = &S.fun;
      S s;
      fp(&s, 2);
      import std.stdio;
      writeln(s.a);
   }

When I put the ctx pointer at the end, and called fp(2, &s) it worked. I deduced from this that the compiler generates code that thinks the context pointer is at the end of the parameter list.

[ibuclaw]

@RazvanN7 looks like you're trying to do X86 ABI hacks in the front-end AST then. What you're doing won't work - see for example #13590 and the decades of grief that caused us in order to get opCmp and opEquals working uniformly across all architectures.

Remember, X86 ABI pushes arguments RTL, contrary to every single other platform which pushes LTR.

[RazvanN7]

@ibuclaw Thanks for shedding some light on this. It seems that the frontend doesn't really have a way to express a function type for which the context pointer is provided by the user (other than delegates.funcptr).

[Bolpat]

My two cents:

The first thing to consider is that &S.f is easily written with the intention of &s.f (with s an instance of S), thus making it an error provides indeed value. In simple cases, programmers can write out function(ref S _this, …) => _this.f(…) explicitly, but we can provide a mechanism for that in Phobos or via __traits, e.g. __traits(explicitObjectParameter, S.f).

Assuming &S.f is not made a compile error:

• The instance parameter must be the first parameter. Everything else surprises programmers with experience in e.g. Extension Methods in C# and Explicit Object Parameters in C++, which put the instance parameter first. It also needlessly complicates meta-programming, especially when sequencing the instance parameter before a variadic parameter (if any).
• The instance parameter should be what it is in a member function: For a struct or union, that is a reference, not a pointer. It has no business being null or having pointer arithmetic done to it. For a class, the instance parameter should be a class handle (which is sadly nullable in D).

The syntax &S.f can be a shorthand for function(ref S _this, …) => _this.f(…) (where … is the parameters of f), but it has the downside that the address might not be stable, i.e. might give different values when the pointer is converted to size_t.
If that is desired, D could automatically define them as module-level functions that correspond to non-static member functions, but take the object parameter explicitly. Those can be hidden or exposed for intentional use by regular programming. Exposing them has an interesting consequence:
The module S is in has a function f with the instance parameter as the first parameter. This would allow true UFCS: A member function can now be called as if it were a free function! For virtual class methods, this must forward to a member function call so overriding works.

I’d imagine the module-level function implementation as follows: For every aggregate type T defined in a module and every non-static member function f defined in T generate module-level __T_f with the same visibility and return type as f, and parameters are the instance parameter plus the parameters of f; also generate module-level alias f = __T_f.
The syntax &S.f becomes &__S_f (where __S_f is searched for in the module S is defined in). When multiple types S1, S2, … define a non-static member functions f, module-level f is an overload set spanning all these functions, but &S1.f, &S2.f, … already disambiguate by the first type, and target-type overload resolution is not needed for those that define exactly one overload.