Fix Issues 23408 and 23403 - FUNCTION does not resolve properly #14549

RazvanN7 · 2022-10-12T10:47:38Z

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 · 2022-10-12T10:47:42Z

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"

RazvanN7 · 2022-10-12T10:50:01Z

compiler/test/runnable/test18916.d

@@ -11,7 +11,7 @@ struct Line

 void foo(Line line1 = __LINE__, int line2 = __LINE__, int line3 = int(__LINE__))
 {
-    assert(line1 == 12);
+    assert(line1 == 21);
    assert(line2 == 21);
    assert(line3 == 12);


This can also be modified to print 21, however, according to @WalterBright in this comment this sort of construction is used to retain the C++ behavior of LINE

We also have bugs about __FUNCTION__.ptr not resolving correctly so I don't think that's a good argument.

Geod24 · 2022-10-12T11:59:27Z

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 · 2022-10-12T22:02:07Z

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

ibuclaw · 2022-10-13T02:40:38Z

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 · 2022-10-13T03:07:01Z

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 · 2022-10-13T03:21:07Z

Now targeting stable.

RazvanN7 · 2022-10-13T10:39:44Z

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

dkorpel · 2022-10-13T11:15:47Z

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 · 2022-10-13T17:34:11Z

I looked at what's blocked because of this, and the test case added in this PR is the culprit:
#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%.

#14550 can be unblocked by simply modifying the test case.

RazvanN7 · 2022-10-14T03:56:00Z

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 · 2022-10-14T23:41:18Z

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 · 2022-10-17T04:15:28Z

@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 · 2022-10-17T08:53:43Z

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 · 2022-10-17T09:39:40Z

If we can get #14309 in, then semantic analysis is done for default arguments at each call site, which may also help with fixing this issue.

pbackus · 2022-10-17T12:33:54Z

@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."

WalterBright · 2022-11-05T07:46:49Z

compiler/src/dmd/expression.d

+    {
+        for (int i = 0; i < (*elements).length; i++)
+        {
+            auto elem = (*elements)[i];


foreach (ref elem; (*elements)[])
if (elem)
elem = elem.resolveLoc(loc, sc);

WalterBright · 2022-11-05T07:47:24Z

compiler/src/dmd/expression.d

+            auto arg = (*arguments)[i];
+            if (arg)
+                (*arguments)[i] = arg.resolveLoc(loc, sc);
+        }


RazvanN7 · 2022-11-09T08:43:44Z

@WalterBright I take it you agree with this addition?

ibuclaw · 2023-01-15T15:04:15Z

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

RazvanN7 · 2023-01-16T09:18:04Z

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

dlang-bot added the Severity:Bug Fix label Oct 12, 2022

RazvanN7 commented Oct 12, 2022

View reviewed changes

RazvanN7 force-pushed the Issue_23403 branch from a2ccf2c to 815f915 Compare October 13, 2022 03:18

RazvanN7 changed the base branch from master to stable October 13, 2022 03:18

RazvanN7 closed this Oct 13, 2022

RazvanN7 reopened this Oct 13, 2022

RazvanN7 force-pushed the Issue_23403 branch from 815f915 to efaf7ef Compare October 13, 2022 03:31

Fix Issues 23408 and 23403 - __FUNCTION__ does not resolve properly

2927621

RazvanN7 force-pushed the Issue_23403 branch from efaf7ef to 2927621 Compare October 13, 2022 03:55

RazvanN7 requested a review from ibuclaw as a code owner October 13, 2022 03:55

RazvanN7 added the Review:Blocking Other Work review and pulling should be a priority label Oct 13, 2022

dkorpel mentioned this pull request Oct 13, 2022

Translate _d_arraycatnTX to a template #14550

Merged

dlang-bot added the Review:stable-stalled label Oct 20, 2022

WalterBright reviewed Nov 5, 2022

View reviewed changes

compiler/src/dmd/expression.d

auto arg = (*arguments)[i];

if (arg)

(*arguments)[i] = arg.resolveLoc(loc, sc);

}

Copy link

Member

WalterBright Nov 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

dlang-bot added Review:stable-stalled and removed Review:stable-stalled labels Nov 5, 2022

dlang-bot added Review:stable-stalled and removed Review:stable-stalled labels Nov 9, 2022

dlang-bot added Review:stable-stalled and removed Review:stable-stalled labels Jan 17, 2023

dlang-bot added the Review:stalled label Apr 16, 2023

dlang-bot added the Review:Needs Rebase label Nov 2, 2023

pbackus mentioned this pull request Dec 31, 2023

Fix issue 18919 - __FILE__ and __LINE__ should work when used in defa… #15968

Merged

RazvanN7 closed this Jun 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issues 23408 and 23403 - FUNCTION does not resolve properly #14549

Fix Issues 23408 and 23403 - FUNCTION does not resolve properly #14549

RazvanN7 commented Oct 12, 2022

dlang-bot commented Oct 12, 2022 •

edited

Loading

RazvanN7 Oct 12, 2022

Geod24 Oct 12, 2022

Geod24 commented Oct 12, 2022

dkorpel commented Oct 12, 2022

ibuclaw commented Oct 13, 2022

RazvanN7 commented Oct 13, 2022 •

edited

Loading

RazvanN7 commented Oct 13, 2022

RazvanN7 commented Oct 13, 2022

dkorpel commented Oct 13, 2022

dkorpel commented Oct 13, 2022

RazvanN7 commented Oct 14, 2022

pbackus commented Oct 14, 2022

RazvanN7 commented Oct 17, 2022

dkorpel commented Oct 17, 2022

dkorpel commented Oct 17, 2022

pbackus commented Oct 17, 2022

WalterBright Nov 5, 2022

WalterBright Nov 5, 2022

RazvanN7 commented Nov 9, 2022

ibuclaw commented Jan 15, 2023

RazvanN7 commented Jan 16, 2023

Fix Issues 23408 and 23403 - __FUNCTION__ does not resolve properly #14549

Fix Issues 23408 and 23403 - __FUNCTION__ does not resolve properly #14549

Conversation

RazvanN7 commented Oct 12, 2022

dlang-bot commented Oct 12, 2022 • edited Loading

Bugzilla references

Testing this PR locally

RazvanN7 Oct 12, 2022

Choose a reason for hiding this comment

Geod24 Oct 12, 2022

Choose a reason for hiding this comment

Geod24 commented Oct 12, 2022

dkorpel commented Oct 12, 2022

ibuclaw commented Oct 13, 2022

RazvanN7 commented Oct 13, 2022 • edited Loading

RazvanN7 commented Oct 13, 2022

RazvanN7 commented Oct 13, 2022

dkorpel commented Oct 13, 2022

dkorpel commented Oct 13, 2022

RazvanN7 commented Oct 14, 2022

pbackus commented Oct 14, 2022

RazvanN7 commented Oct 17, 2022

dkorpel commented Oct 17, 2022

dkorpel commented Oct 17, 2022

pbackus commented Oct 17, 2022

WalterBright Nov 5, 2022

Choose a reason for hiding this comment

WalterBright Nov 5, 2022

Choose a reason for hiding this comment

RazvanN7 commented Nov 9, 2022

ibuclaw commented Jan 15, 2023

RazvanN7 commented Jan 16, 2023

Fix Issues 23408 and 23403 - FUNCTION does not resolve properly #14549

Fix Issues 23408 and 23403 - FUNCTION does not resolve properly #14549

dlang-bot commented Oct 12, 2022 •

edited

Loading

RazvanN7 commented Oct 13, 2022 •

edited

Loading