dmd icon indicating copy to clipboard operation
dmd copied to clipboard

Fix 22540 - Copy parameter types / defaults of dependant template...

Open MoonlightSentinel opened this issue 2 years ago • 3 comments

... value / alias parameters.

The introduced copies ensure that the initialized parameters of the instantiation do not share objects with the template declaration.

Previously, sharing could occur in the following scenarios:

  • type parameter appears inside of value type, e.g. foo(T, T* val) Later semantic resolves T to the concrete type and hence changes the type of val in the template declaration.
  • alias parameter default value isn't copied and manifested according to the first template instance.

TODO:

  • [ ] Investigate https://issues.dlang.org/show_bug.cgi?id=22526 which seems related

MoonlightSentinel avatar Nov 23 '21 21:11 MoonlightSentinel

Thanks for your pull request and interest in making D better, @MoonlightSentinel! 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
22540 critical Instantiation modifies dependant type of value / alias template parameters

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

dlang-bot avatar Nov 23 '21 21:11 dlang-bot

In that case it’s still worth making it obvious  From: FlorianSent: 23 November 2021 23:08To: dlang/dmdCc: Max Haughton; CommentSubject: Re: [dlang/dmd] Fix 22540 - Copy parameter types / defaults of dependant template... (PR ***@***.*** commented on this pull request.In src/dmd/dtemplate.d:> @@ -8039,7 +8043,16 @@ MATCH matchArg(TemplateParameter tp, Scope* sc, RootObject oarg, size_t i, Templ         }          //printf("\tvalType: %s, ty = %d\n", tvp.valType.toChars(), tvp.valType.ty);-        vt = tvp.valType.typeSemantic(tvp.loc, sc);++        // The type might rely on preceeding parameters, e.g.+        // void foo(T, T val)(...) {}+        // Make a copy to not modify the template declaration!but this smells of something really broken in the sema code somewhere so best make it super obvious.Not necessarily, the assumption for template instances is to work on an independent copy of the AST that can be modified by semantic. The code previously didn't make a copy at all.Making the copy conditional is mostly intended to avoid redundant copies.—You are receiving this because you commented.Reply to this email directly, view it on GitHub, or unsubscribe.Triage notifications on the go with GitHub Mobile for iOS or Android.  

maxhaton avatar Nov 23 '21 23:11 maxhaton

@MoonlightSentinel what should we do with this

maxhaton avatar Feb 02 '22 02:02 maxhaton

Closing as draft appears to be abandoned.

ibuclaw avatar Jan 15 '23 14:01 ibuclaw