Fix issue 18919 - FILE and LINE should work when used in defa… #15968

tim-dlang · 2023-12-30T10:56:25Z

…ult argument expressions

The parser now always creates AST nodes for default init expressions like __FILE__. They are replaced in resolveLoc. Variable inDefaultArg in Scope is used, so the nodes are not replaced too early.

dlang-bot · 2023-12-30T10:56:31Z

Thanks for your pull request and interest in making D better, @tim-dlang! 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
✓	18919	enhancement	__FILE__ and __LINE__ should work when used in default argument expressions

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

compiler/src/dmd/dscope.d

changelog/dmd.default-init.dd

ghost · 2023-12-30T11:14:16Z

changelog/dmd.default-init.dd

+   size_t line;
+}
+
+void func2(Loc loc = Loc(__FILE__, __LINE__))


as a code comment you should mention that this case in particular did not work

changelog/dmd.default-init.dd

ghost

The changelog entry needs to be more accurate.

tim-dlang · 2023-12-30T12:55:41Z

The new test was not portable, because the output contained path separators and size_t was different. Now the output is more portable.

I also fixed another edge case: CatExp is lowered to _d_arraycatnTX, but __FILE__ inside the lowering was not changed. Function resolveLoc now also runs for the lowering.

tim-dlang · 2023-12-30T13:48:15Z

Pipeline "Windows_VisualD_LDC x86-mscoff_MinGW" seems to not support %zd for printf, so the test now only uses %d and int for line numbers.

A memory allocation failure on "Windows_VisualD_LDC x64_Debug" seems to also randomly happen on other PRs. I have created an issue for that: https://issues.dlang.org/show_bug.cgi?id=24309

schveiguy · 2023-12-30T21:23:57Z

So cool, thanks!

dkorpel · 2023-12-30T21:29:16Z

What happens now if you do f(int x = someTemplate!__LINE__)?

maxhaton · 2023-12-30T21:53:34Z

compiler/src/dmd/expressionsem.d

+        const saveInDefaultArg = sc.inDefaultArg;
+        sc.inDefaultArg = false;
+
        auto e = compileIt(exp);
+        sc.inDefaultArg = saveInDefaultArg;


It's not clear to me what's going on here

This is relevant for the following test:

void func(string expr = mixin("\"expr2=" ~ __MODULE__ ~ "\"")) {}

The mixin expression is inside a default argument, so sc.inDefaultArg is true. The code now sets sc.inDefaultArg temporarily to false, so the inside of the expression can be evaluated at compile time.
It's the same problem as for the example by @dkorpel with a template, but I only fixed it for a mixin expression and not templates.

compiler/src/dmd/expressionsem.d

maxhaton · 2023-12-30T21:57:16Z

compiler/test/runnable/issue18919.d

+import imports.issue18919b;
+
+void main()
+{


Could this be a compilable? (They're much faster).

No, the test checks the output of the program at runtime und RUN_OUTPUT.

A lot of the compilables use a static assert to check values.

The values are not always available at compile time. Only func5 and func6 pass the values as template parameters, so they could be checked with static assert. Most of the test functions pass the values using runtime parameters, so a test at runtime is still necessary.

Maybe it would be possible to run everything at CTFE. Then it could be in compilable. But then it would not test that for example __FILE__.ptr is correctly handled by the backend.

tim-dlang · 2023-12-30T21:57:41Z

What happens now if you do f(int x = someTemplate!__LINE__)?

Unfortunately that is now an error:

Error: cannot interpret `__LINE__` at compile time

I did not check this previously. It would be possible to change it, so it still compiles, but it would be the line at the template instantiation and not the call site.

schveiguy · 2023-12-30T22:20:48Z

If you aren't going to "fix" the template thing, then the current behavior should be preserved. It should not become an error.

I would say fixing template parameters to be from the call site is not a requirement for this issue. But certainly, we can't break existing code.

tim-dlang · 2023-12-30T22:45:41Z

The example f(int x = someTemplate!__LINE__) now works again, but uses the value at the location of the template instantiation.

pbackus · 2023-12-31T01:06:42Z

The fact that we have a special case in the language for __FILE__ and __LINE__ as default arguments is not a good thing, and expanding this special case further is a move in the wrong direction. See the discussion in PR #14549 starting from this comment for a more detailed explanation.

I think a better way to support these use-cases would be to add a general-purpose mechanism for evaluating default arguments at the call site, which is not tied specifically to the presence of the __FILE__ or __LINE__ tokens.

schveiguy · 2023-12-31T02:45:12Z

I think a better way to support these use-cases would be to add a general-purpose mechanism for evaluating default arguments at the call site, which is not tied specifically to the presence of the __FILE__ or __LINE__ tokens.

I don't disagree, but a couple questions here:

Is this easy to do?
Is this possible to do later if we merge this now?

I don't want perfect to be the enemy of good here. We can make the code look nice later, right?

This is a real use case that can immediately be useful, since 99.999% of people create exceptions without specifying explicitly the line and file, they let the default arguments do their thing. We can easily deprecate the size_t and string parameters, and use some LOC struct to fix this, and then it secures infinitely more the intent of the user when building exceptions using explicit file/line parameters. I would hate to delay it because the implementation details are not perfect.

pbackus · 2023-12-31T02:55:52Z

Is this easy to do?

I don't think it's a huge challenge technically, but it probably requires a DIP

Is this possible to do later if we merge this now?

Yes, but then we're stuck with the special case.

That said, there's a case to be made that having __FILE__ and __LINE__ always behave like this is less of a special case then having them only behave that way at the top level, and I can't deny that it's useful. So maybe we should merge this now, and look into replacing it with a more general solution in a future language edition.

tim-dlang · 2023-12-31T10:38:39Z

Currently, the semantic analysis for default arguments runs once at the declaration site. Later the AST for a default argument is copied to the call site and special tokens are replaced. This is not only done for the special tokens, but also for code like f(Class obj = new Class), where a new object is created for every call, or f(void* buffer = alloca(200)), where memory is allocated on the stack, which has to be valid for the calling function. This even works with more complex expressions.

The compiler has another special case before this PR for function void f(const(char)* file=__FILE__.ptr, const(char)* func=__FUNCTION__.ptr) . Here __FUNCTION__ is evaluated at the call site, but __FILE__ is evaluated at the declaration site. My PR implements all special tokens the same, so they are consistent.

A more general solution could be to run the semantic analysis for default arguments not just once, but for every call of a function. Symbol resolution should still be done in the context of the function declaration. Templates could then be instantiated with different values for special tokens as template arguments. This could also be more maintainable, because resolveLoc would not be needed anymore. Currently, resolveLoc would need to be updated for some changes to AST classes. On the other hand, running semantic analysis multiple times could be slower.

PetarKirov · 2024-01-01T09:15:49Z

A more general solution could be to run the semantic analysis for default arguments not just once, but for every call of a function. [..]

This indeed sounds like a more general and principled approach to me.

[..] On the other hand, running semantic analysis multiple times could be slower.

This worries me as well.

That's why I think should try out this approach and measure the performance impact. And then take a decision based on the data.

tim-dlang · 2024-01-02T16:56:19Z

That's why I think should try out this approach and measure the performance impact. And then take a decision based on the data.

I did a first test of doing the semantic analysis at the call site, but it is not complete, and this PR is unchanged. I tested it by compiling std.regex with unittests enabled.

Doing the semantic analysis only at the call site increased the runtime by roughly 0.6%. Unfortunately the semantic analysis also needs to be done at the declaration site, so error messages can be shown for functions never used. Doing the semantic analysis both at the declaration site and the call site solves this, but then the runtime of DMD increases by roughly 4.9%.

Another problem is memory consumption: The scope of the function needs to remain valid, so it can be used by the semantic analysis at the call site. In my test DMD consumed 2.1% more memory.

It was only a first test and there could be possible optimizations, or I made mistakes, but I think doing semantic analysis at the call site would not be worth it with those increases in runtime and memory consumption.

dkorpel

As others have mentioned, this isn't particularly ideal. I also wish we could re-use existing visitor code for this. However, this has immediate value, and I agree with Steven:

I would hate to delay it because the implementation details are not perfect.

So I think it's good to get this in.

dkorpel · 2024-01-07T11:18:57Z

compiler/src/dmd/expressionsem.d

+        return exp;
+    }
+
+    Expression visitArray(ArrayExp exp)


This function is not covered by tests

dkorpel · 2024-01-07T11:25:00Z

compiler/src/dmd/dtemplate.d

@@ -5625,7 +5625,11 @@ extern (C++) final class TemplateValueParameter : TemplateParameter
        if (e)
        {
            e = e.syntaxCopy();
-            if ((e = e.expressionSemantic(sc)) is null)
+            const saveInDefaultArg = sc.inDefaultArg;
+            sc.inDefaultArg = true;


This 'save and restore' pattern is risky, since sc may be referenced by siblings. It's safer to push and pop a scope, see: #15434 (comment)

Although in these cases, I don't think it's possible for there to be expression siblings that aren't default args.

It's safer to push and pop a scope

Welp, that aged well 😆

#16781

dkorpel · 2024-01-07T11:30:20Z

compiler/src/dmd/parse.d

-     * Parses default argument initializer expression that is an assign expression,
-     * with special handling for __FILE__, __FILE_DIR__, __LINE__, __MODULE__, __FUNCTION__, and __PRETTY_FUNCTION__.
-     */
-    private AST.Expression parseDefaultInitExp()


I love this removal

dkorpel · 2024-01-07T11:31:03Z

compiler/src/dmd/parse.d


        case TOK.line:
-            e = new AST.IntegerExp(loc, loc.linnum, AST.Type.tint32);


And I love how the tokens aren't resolved in the parser anymore, which is useful for dmd as a library

…ult argument expressions The parser now always creates AST nodes for default init expressions like __FILE__. They are replaced in resolveLoc. Variable inDefaultArg in Scope is used, so the nodes are not replaced too early.

tim-dlang · 2024-01-07T13:27:28Z

I have added a test for ArrayExp and modified the code changing inDefaultArg, so it uses a new scope.

Unfortunately this change first triggered an assert in Scope.push for test compilable/test9701.d: The assert checks that the new scope is not the current scope. Scope.push was called inside Scope.startCTFE, which was called inside getMessage(DeprecatedDeclaration dd). It was trying to use a scope, which was already freed.

I added the call em.depdecl._scope.setNoFree(); for deprecated enum members, so this does not happen.

RazvanN7 · 2024-05-24T09:51:06Z

This PR introduced a regression: https://issues.dlang.org/show_bug.cgi?id=24560

Fixed in: #16519

dlang-bot added the Severity:Enhancement label Dec 30, 2023

thewilsonator reviewed Dec 30, 2023

View reviewed changes

compiler/src/dmd/dscope.d Show resolved Hide resolved

tim-dlang force-pushed the issue18919 branch from 8c6bce6 to 3d8835c Compare December 30, 2023 11:07

tim-dlang requested a review from ibuclaw as a code owner December 30, 2023 11:07

ghost reviewed Dec 30, 2023

View reviewed changes

changelog/dmd.default-init.dd Outdated Show resolved Hide resolved

ghost reviewed Dec 30, 2023

View reviewed changes

changelog/dmd.default-init.dd Outdated Show resolved Hide resolved

ghost reviewed Dec 30, 2023

View reviewed changes

changelog/dmd.default-init.dd Outdated Show resolved Hide resolved

ghost suggested changes Dec 30, 2023

View reviewed changes

tim-dlang force-pushed the issue18919 branch 2 times, most recently from 13a2efd to 5e1f5f2 Compare December 30, 2023 12:53

tim-dlang force-pushed the issue18919 branch from 5e1f5f2 to 480f2cb Compare December 30, 2023 13:25

maxhaton reviewed Dec 30, 2023

View reviewed changes

compiler/src/dmd/expressionsem.d Outdated Show resolved Hide resolved

maxhaton reviewed Dec 30, 2023

View reviewed changes

tim-dlang force-pushed the issue18919 branch 2 times, most recently from 58e9f2d to 56a42ed Compare December 30, 2023 22:45

dkorpel approved these changes Jan 7, 2024

View reviewed changes

tim-dlang force-pushed the issue18919 branch from 56a42ed to b2b8106 Compare January 7, 2024 13:25

dkorpel merged commit 33286cc into dlang:master Jan 7, 2024
46 checks passed

tim-dlang deleted the issue18919 branch January 8, 2024 16:41

RazvanN7 mentioned this pull request May 24, 2024

Fix Bugzilla Issue 24560 - dmd crash on imported function with default parameter containing new #16519

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 18919 - FILE and LINE should work when used in defa… #15968

Fix issue 18919 - FILE and LINE should work when used in defa… #15968

tim-dlang commented Dec 30, 2023 •

edited by schveiguy

Loading

dlang-bot commented Dec 30, 2023

ghost Dec 30, 2023 •

edited by ghost

Loading

ghost left a comment

tim-dlang commented Dec 30, 2023

tim-dlang commented Dec 30, 2023

schveiguy commented Dec 30, 2023

dkorpel commented Dec 30, 2023

maxhaton Dec 30, 2023

tim-dlang Dec 30, 2023

maxhaton Dec 30, 2023

tim-dlang Dec 30, 2023

maxhaton Dec 30, 2023

tim-dlang Dec 30, 2023

tim-dlang Dec 31, 2023

tim-dlang commented Dec 30, 2023

schveiguy commented Dec 30, 2023 •

edited

Loading

tim-dlang commented Dec 30, 2023

pbackus commented Dec 31, 2023

schveiguy commented Dec 31, 2023 •

edited

Loading

pbackus commented Dec 31, 2023

tim-dlang commented Dec 31, 2023

PetarKirov commented Jan 1, 2024

tim-dlang commented Jan 2, 2024

dkorpel left a comment

dkorpel Jan 7, 2024

dkorpel Jan 7, 2024

dkorpel Aug 13, 2024

dkorpel Jan 7, 2024

dkorpel Jan 7, 2024 •

edited

Loading

tim-dlang commented Jan 7, 2024

RazvanN7 commented May 24, 2024


		case TOK.line:
		e = new AST.IntegerExp(loc, loc.linnum, AST.Type.tint32);

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

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

Conversation

tim-dlang commented Dec 30, 2023 • edited by schveiguy Loading

dlang-bot commented Dec 30, 2023

Bugzilla references

Testing this PR locally

ghost Dec 30, 2023 • edited by ghost Loading

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

tim-dlang commented Dec 30, 2023

tim-dlang commented Dec 30, 2023

schveiguy commented Dec 30, 2023

dkorpel commented Dec 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tim-dlang commented Dec 30, 2023

schveiguy commented Dec 30, 2023 • edited Loading

tim-dlang commented Dec 30, 2023

pbackus commented Dec 31, 2023

schveiguy commented Dec 31, 2023 • edited Loading

pbackus commented Dec 31, 2023

tim-dlang commented Dec 31, 2023

PetarKirov commented Jan 1, 2024

tim-dlang commented Jan 2, 2024

dkorpel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkorpel Jan 7, 2024 • edited Loading

Choose a reason for hiding this comment

tim-dlang commented Jan 7, 2024

RazvanN7 commented May 24, 2024

Fix issue 18919 - FILE and LINE should work when used in defa… #15968

Fix issue 18919 - FILE and LINE should work when used in defa… #15968

tim-dlang commented Dec 30, 2023 •

edited by schveiguy

Loading

ghost Dec 30, 2023 •

edited by ghost

Loading

schveiguy commented Dec 30, 2023 •

edited

Loading

schveiguy commented Dec 31, 2023 •

edited

Loading

dkorpel Jan 7, 2024 •

edited

Loading