Fix Issues 23408 and 23403 - __FUNCTION__ does not resolve properly

[RazvanN7]

Properly resolves __FUNCTION__ and folks by adding resolveLoc for StructLiteralExp and CallExp. The new resolveLoc overrides call resolveLoc on the arguments of the 2 AST constructions.

I am not targeting stable because this is blocking @teodutu from implementing the array concatenation template hook and lowering (which target master).

[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
✓	23403	critical	Segfault when calling auto-generated struct constuctor with __FUNCTION__ or __PRETTY_FUNCTION__
✓	23408	blocker	__FUNCTION__ does not resolve correctly

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 "stable + dmd#14549"

[Geod24]

I am not targeting stable because this is blocking @teodutu from implementing the array concatenation template hook and lowering (which target master).

Nonsense, just merge stable back into master.

[dkorpel]

I'm all for properly resolving special tokens in default arguments, but why only do it for struct literals and call expressions?

[ibuclaw]

I am not targeting stable because this is blocking @teodutu from implementing the array concatenation template hook and lowering (which target master).

You can always merge stable to master immediately afterwards.

[RazvanN7]

Nonsense, just merge stable back into master.

Fair enough.

I'm all for properly resolving special tokens in default arguments, but why only do it for struct literals and call expressions?

Those are the ones I've identified so far. We can incrementally add them as soon as we identify them.

You can always merge stable to master immediately afterwards.

OK. I just hoped I could avoid doing that. Don't want to deal with potential merge conflicts.

[RazvanN7]

Now targeting stable.

[RazvanN7]

Anything else blocking this? I wish we could move on with this so that we can unblock other work.

[dkorpel]

I don't like how this PR appears to be about solving a bug with __FUNCTION__, but it's actually shifting the way how special tokens are resolved in default arguments.

Those are the ones I've identified so far. We can incrementally add them as soon as we identify them.

Special tokens can be part of any expression.

void foo(string f = toLower(__FUNCTION__[0] ~ "()") );
void foo(int f = (__LINE__ > 10) ? 3 : 5);

It's pretty obvious, cases are not waiting to be identified. Currently special tokens are deliberately only resolved in the top level, and not when part of a larger expression. This PR moves away from 'shallow' resolution to 'deep' resolution, but only 10% there.

If the plan is to move 100% there, then @WalterBright should give green light since he objected to that in the comment you linked. If we go through with it, an expression visitor might be re-used instead of implementing resolveLoc in all Expression nodes. Also, when special tokens are assigned to template parameters, type checking of default arguments cannot be done from the function signature anymore, but only at the call site.

void f(int x = T!__LINE__); // is the type correct? Depends on where it's called from.

[dkorpel]

I looked at what's blocked because of this, and the test case added in this PR is the culprit: https://github.com/dlang/dmd/pull/9377

I wasn't aware you implemented resolveLoc for binary expressions before to fix a similar issue. The 'shallow' <> 'deep' needle doesn't move from 0% to 10%, but more like 10% -> 20%. I'd rather not go more towards 'deep' unless we fully commit to 100%.

https://github.com/dlang/dmd/pull/14550 can be unblocked by simply modifying the test case.

[RazvanN7]

I would argue we should commit to 100%. The spec currently does not specify anything about shallow vs. deep, rather that FUNCTION and other special function are evaluated at the point of instantiation. So, I think that most users expectation would be the behavior that this PR is introducing. Anyway, currently, if you use FUNCTION nested in other constructs you either get an unexpected result or a compiler crash.

[pbackus]

IMO it would be much better to either move the proverbial "needle" back to 0% and keep it there, or commit to having all default arguments evaluated at the call site, regardless of whether they contain a special keyword like __FUNCTION__.

The middle-ground approach, where we have both kinds of evaluation in the language and the switch to select between them is "does this expression's AST contain a special keyword anywhere", is going to result in a rat's nest of edge-case interactions (some of which @dkorpel has already pointed out), and will encourage D programmers to invent "idioms" like the following:

// Use `readln()` as default argument
// Evaluated at call site due to inclusion of __LINE__
auto fun(string s = () { if (__LINE__) return readln(); }())
{
    // ...
}

This is exactly the sort of thing Andrei was referring to when he coined the term "Good Work". It's the sort of thing that makes D more like C++—full of obscure, dark corners that only compiler developers and obsessive readers of the language spec fully understand. I hope we can agree that this is not a path we want to go down.

[RazvanN7]

@pbackus I think that keeping the needle at 0% is not the behavior users expect. This causes reports such as [1][2] and many other duplicated/sightly modified examples. The current design (if I may call it so, because it looks like unintended behavior rather than a design choice) is surprising and borderline useless. By looking at the minimally provided spec [3] ("The attributes of the AssignExpression are applied where the default expression is used." - but of course the provided example has always compiled), it seems that doing the analysis of default arguments at the call site is what was intended, but not implemented. Also, there is [4] ("FUNCTION expands to the fully qualified name of the function at the point of instantiation.").

Since we must evaluate default arguments at the call site in order to check for attributes, how can we make a case for the special tokens to be evaluated at the call site only when they are not nested in other expressions? This does not make any sense to me. The situation that we have now is that you can break the type system through default arguments, if you want to fix that, you need to evaluate the default arguments at the call site, otherwise there is no way of fixing [1].

In my view, this is just a small fix to get things in the right direction. Or we can just drop this PR and keep this broken state, but let's not argue that the current "design" is what's intended and the correct way.

[1] https://issues.dlang.org/show_bug.cgi?id=11048 [2] https://issues.dlang.org/show_bug.cgi?id=18919 [3] https://dlang.org/spec/function.html#function-default-args [4] https://dlang.org/spec/expression.html#specialkeywords

[dkorpel]

In my view, this is just a small fix to get things in the right direction. Or we can just drop this PR and keep this broken state, but let's not argue that the current "design" is what's intended and the correct way.

I don't think anyone is arguing that, I'm all for going 100%. But this PR adds a little more top-down analysis which Walter is against. What if we resolved DefaultInitExp during semantic3 based on Scope? Then it wouldn't be top-down, but you still get proper resolution.

[dkorpel]

If we can get https://github.com/dlang/dmd/pull/14309 in, then semantic analysis is done for default arguments at each call site, which may also help with fixing this issue.

[pbackus]

@RazvanN7 If the plan is to ultimately evaluate all default arguments at the call site, that's fine. The outcome I want to avoid is "some default arguments are evaluated at the call site, some are evaluated at the definition site, and the rule for which is which is complex and difficult to document/teach."

[RazvanN7]

@WalterBright I take it you agree with this addition?

[ibuclaw]

@RazvanN7 still pursuing or found another way to unblock the template work?

[RazvanN7]

@ibuclaw The template work is unblocked. If I understand correctly, #14309 is going to make this redundant.