Fix Issue 7619 - Infer deprecated for template instances #10677

MoonlightSentinel · 2019-12-18T18:01:54Z

Template instances may use deprecated symbols that were passed as
template parameters or selected based on an instance-specific condition.
Previously DMD raised deprecation messages for the deprecated symbol at
the point of instantiation AND substitution inside of the template code.
The latter is cannot be suppressed by using deprecated because that
will only apply to certain instantiations.

This commit changes the behavior to check whether a deprecated symbol
is used inside of a template instance and marks the instance as
deprecated if found. That way only the actual usage of the instance
will raise deprecation messages.

Ideally this should only apply to deprecated symbols that depend on the
template parameters but those dependencies are not traceable in general
(static if, compiles, ...).

The current behaviour blocks dlang/phobos#7252 because -checkaction=context may instantiate templates with deprecated types (e.g. cfloat) in deprecated unittests.

MoonlightSentinel · 2020-01-03T13:54:24Z

@Geod24 Could you have a look?

MoonlightSentinel · 2020-01-11T22:22:06Z

Is there a way to detect whether a symbol depends on a template parameter?
E.g. that a and b are different in the following example:

deprecated struct S {}

void foo(T)()
{
    T a;
    S b;
}

alias bar = foo!S;

Geod24 · 2020-01-12T10:36:05Z

Is there a way to detect whether a symbol depends on a template parameter?

The resolution happens quite early. The TemplateInstance installs the correct types as aliases in the scope (

dmd/src/dmd/dtemplate.d

Lines 2237 to 2330 in 49dfbe5

    
               /************************************************** 
        
                * Declare template parameter tp with value o, and install it in the scope sc. 
        
                */ 
        
               RootObject declareParameter(Scope* sc, TemplateParameter tp, RootObject o) 
        
               { 
        
                   //printf("TemplateDeclaration.declareParameter('%s', o = %p)\n", tp.ident.toChars(), o); 
        
                   Type ta = isType(o); 
        
                   Expression ea = isExpression(o); 
        
                   Dsymbol sa = isDsymbol(o); 
        
                   Tuple va = isTuple(o); 
        
                   Declaration d; 
        
                   VarDeclaration v = null; 
        
                   if (ea && ea.op == TOK.type) 
        
                       ta = ea.type; 
        
                   else if (ea && ea.op == TOK.scope_) 
        
                       sa = (cast(ScopeExp)ea).sds; 
        
                   else if (ea && (ea.op == TOK.this_ || ea.op == TOK.super_)) 
        
                       sa = (cast(ThisExp)ea).var; 
        
                   else if (ea && ea.op == TOK.function_) 
        
                   { 
        
                       if ((cast(FuncExp)ea).td) 
        
                           sa = (cast(FuncExp)ea).td; 
        
                       else 
        
                           sa = (cast(FuncExp)ea).fd; 
        
                   } 
        
                   if (ta) 
        
                   { 
        
                       //printf("type %s\n", ta.toChars()); 
        
                       auto ad = new AliasDeclaration(Loc.initial, tp.ident, ta); 
        
                       ad.wasTemplateParameter = true; 
        
                       d = ad; 
        
                   } 
        
                   else if (sa) 
        
                   { 
        
                       //printf("Alias %s %s;\n", sa.ident.toChars(), tp.ident.toChars()); 
        
                       auto ad = new AliasDeclaration(Loc.initial, tp.ident, sa); 
        
                       ad.wasTemplateParameter = true; 
        
                       d = ad; 
        
                   } 
        
                   else if (ea) 
        
                   { 
        
                       // tdtypes.data[i] always matches ea here 
        
                       Initializer _init = new ExpInitializer(loc, ea); 
        
                       TemplateValueParameter tvp = tp.isTemplateValueParameter(); 
        
                       Type t = tvp ? tvp.valType : null; 
        
                       v = new VarDeclaration(loc, t, tp.ident, _init); 
        
                       v.storage_class = STC.manifest | STC.templateparameter; 
        
                       d = v; 
        
                   } 
        
                   else if (va) 
        
                   { 
        
                       //printf("\ttuple\n"); 
        
                       d = new TupleDeclaration(loc, tp.ident, &va.objects); 
        
                   } 
        
                   else 
        
                   { 
        
                       assert(0); 
        
                   } 
        
                   d.storage_class |= STC.templateparameter; 
        
                   if (ta) 
        
                   { 
        
                       Type t = ta; 
        
                       // consistent with Type.checkDeprecated() 
        
                       while (t.ty != Tenum) 
        
                       { 
        
                           if (!t.nextOf()) 
        
                               break; 
        
                           t = (cast(TypeNext)t).next; 
        
                       } 
        
                       if (Dsymbol s = t.toDsymbol(sc)) 
        
                       { 
        
                           if (s.isDeprecated()) 
        
                               d.storage_class |= STC.deprecated_; 
        
                       } 
        
                   } 
        
                   else if (sa) 
        
                   { 
        
                       if (sa.isDeprecated()) 
        
                           d.storage_class |= STC.deprecated_; 
        
                   } 
        
                   if (!sc.insert(d)) 
        
                       error("declaration `%s` is already defined", tp.ident.toChars()); 
        
                   d.dsymbolSemantic(sc); 
        
                   /* So the caller's o gets updated with the result of semantic() being run on o 
        
                    */ 
        
                   if (v) 
        
                       o = v._init.initializerToExpression(); 
        
                   return o; 
        
               }

).

There is a wasTemplateParameter that was added very recently (a7dbe96), but IMHO it's still a hack to work around the fact that aliases are just considered transparent when they clearly are not.

Geod24 · 2020-01-12T10:42:46Z

I'll have to look deeper to be sure, but instinctively I'd say the result should happen at the Scope level, not by adding something to isDeprecated. Because relying on the message being output somewhere else is quite brittle, and marking the template instance as deprecated would not work either (as TemplateInstance gets reused).

MoonlightSentinel · 2020-01-13T09:21:03Z

Is there a way to detect whether a symbol depends on a template parameter?

The resolution happens quite early. The TemplateInstance installs the correct types as aliases in the scope ...

Thanks, I'll look into that.

I'll have to look deeper to be sure, but instinctively I'd say the result should happen at the Scope level, not by adding something to isDeprecated. Because relying on the message being output somewhere else is quite brittle, and marking the template instance as deprecated would not work either (as TemplateInstance gets reused).

Not sure if i undertsood you correctly, but infering deprecated for the template scope would also hide valid deprecation messages e.g. for the usage of S in the previous example.

I originally chose checkDeprecated because it's the smallest common denominator for all the different things that can be templated. Normally i would agree on the error messages but the change only intends to stop additional checks (the template arguments are evaluated anyway at the callsite)

Geod24 · 2020-01-13T09:40:49Z

Not sure if i undertsood you correctly, but infering deprecated for the template scope would also hide valid deprecation messages e.g. for the usage of S in the previous example.

Which line in the test are you referring to ?

MoonlightSentinel · 2020-01-13T09:44:25Z

Which line in the test are you referring to ?

void foo(T)()
{
    T a;
    S b; // <---
}

Geod24 · 2020-01-13T09:48:32Z

Well if foo is instantiated from a deprecated scope you don't want to show that message do you ?

MoonlightSentinel · 2020-01-13T11:46:50Z

Well if foo is instantiated from a deprecated scope you don't want to show that message do you ?

Ideally yes because the templated symbol is using deprecated stuff while not being deprecated itself.
But inferring deprecated might be necessary to handle deprecated symbols obtained from template parameters, e.g.

void bar(alias f)()
{
    alias F = typeof(f());
    F var = f();
    ....
}

deprecated S func() {}

alias bad = bar!func;

F should not raise deprecation warnings but that would require to track these dependencies and which would probably require a lot of work.

Geod24 · 2020-01-13T13:04:08Z

Ideally yes because the templated symbol is using deprecated stuff while not being deprecated itself.

However it will trigger a deprecation warning. And if the other warning is fixed, this one will show up.
Personally I think triggering a deprecation would be a bad idea. Consider the following:

template Transmogrify (T)
{
    static if (is(typeof(T.foobar)))
        alias Transmogriphy = FooBarTransmogriphier;
    else
        alias Transmogriphy = DefaultTransmogriphier;
}

Now if you deprecate the T with foobar and FooBarTransmogriphier, the deprecation will show twice. Not so annoying you say.
But when this template is called from a deprecated template, suddenly the message becomes redundant. Because during a deprecation, you still want to provide the same functionality (not break user's code), but advise the user to move to another symbol.
When working on libraries, I often ended up deprecating a bunch of symbols that were related. We had to sometimes resort to dirty tricks to avoid triggering a deprecation message in user code (e.g. extern(C) prototypes), because we still wanted to expose the same set of functionalities to user code.

Having some deprecations shows but not other would go against this. If we could do dependency tracking it'd be great, but DMD was not built with this in mind so it's a huge undertaking in itself.

MoonlightSentinel · 2020-01-13T15:07:14Z

Okay, I think I got your point now. Assuming your example above, using Transmogrify!S should issue no warnings if inside a deprecated scope and flag the deprecated template instance otherwise (?).

MoonlightSentinel · 2020-01-14T14:30:43Z

Guess I'll try to infer deprecated for templates as it seems to be the most feasable solution.
It might hide deprecation messages in very rare, hypothetical cases but least does not require a significant rewrite.

Geod24 · 2020-01-19T11:53:39Z

test/fail_compilation/diag14875.d

@@ -67,7 +64,7 @@ deprecated template Vaz(T)
 /*
 TEST_OUTPUT:
 ---
-fail_compilation/diag14875.d(75): Error: static assert:  `0` is false
+fail_compilation/diag14875.d(72): Error: static assert:  `0` is false


Just remove this and use -de ?

Can do but I usually try to keep changes to existing tests as small as possible

Geod24 · 2020-01-19T11:54:45Z

test/fail_compilation/diag14875.d

@@ -36,27 +36,24 @@ template Baz(T)
 /*
 TEST_OUTPUT:
 ---
-fail_compilation/diag14875.d(47): Deprecation: class `diag14875.Dep` is deprecated
-fail_compilation/diag14875.d(51): Deprecation: variable `diag14875.depVar` is deprecated
+fail_compilation/diag14875.d(44): Deprecation: class `diag14875.Dep` is deprecated


Actually looking at the issue, I think we should just merge both test cases, since this does not really test the original issue anymore.

MoonlightSentinel · 2020-01-19T12:54:07Z

frontend.d currently fails because this line is inferred as deprecated:

dmd/src/dmd/frontend.d

Line 147 in 129f88f

versionIdentifiers.each!(VersionCondition.addGlobalIdent);

This originates from this deprecated overload of addGlobalIdent which isn't even used in frontend.d

dmd/src/dmd/cond.d

Lines 471 to 475 in 129f88f

    
           deprecated("Kept for C++ compat - Use the string overload instead") 
        
           static void addGlobalIdent(const(char)* ident) 
        
           { 
        
               addGlobalIdent(ident[0 .. ident.strlen]); 
        
           }

Seems like hasDeprecatedParameters needs to handle overloads somehow ...

12345swordy · 2020-06-11T16:18:34Z

What is going on here?

MoonlightSentinel · 2020-06-11T16:21:43Z

Worked on a lot of other stuff but I want to finish this PR in the future.

Template instances may use deprecated symbols that were passed as template parameters or selected based on an instance-specific condition. Previously DMD raised deprecation messages for the deprecated symbol at the point of instantiation AND substitution inside of the template code. The latter is cannot be suppressed by using `deprecated` because that will only apply to certain instantiations. This commit changes the behavior to check whether a deprecated symbol is used inside of a template instance and marks the instance as `deprecated` if found. That way only the actual usage of the instance will raise deprecation messages. Ideally this should only apply to deprecated symbols that depend on the template parameters but those dependencies are not traceable in general (`static if`, `compiles`, ...).

dlang-bot · 2021-11-18T22:41:14Z

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
✓	7619	normal	Broken deprecated feature with template function

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

RazvanN7 · 2022-03-25T12:55:21Z

@MoonlightSentinel It would be great if would manage to finish this.

MoonlightSentinel changed the title ~~Issue 7619 - Broken deprecated feature with template function~~ Fix Issue 7619 - Broken deprecated feature with template function Dec 19, 2019

MoonlightSentinel force-pushed the depr-template branch 2 times, most recently from 329bf9c to ed3746c Compare December 25, 2019 19:04

This was referenced Dec 25, 2019

Fix issue 11989 - Deprecate core.time.TickDuration dlang/druntime#2886

Closed

Makefiles: Enable -checkaction-context for unittest builds dlang/phobos#7252

Closed

MoonlightSentinel force-pushed the depr-template branch 4 times, most recently from 6723367 to 6bcba3d Compare January 11, 2020 21:26

MoonlightSentinel force-pushed the depr-template branch from 6bcba3d to 48aed92 Compare January 18, 2020 20:04

Geod24 reviewed Jan 19, 2020

View reviewed changes

MoonlightSentinel force-pushed the depr-template branch from be6c2cb to 2a1510e Compare January 30, 2020 13:23

MoonlightSentinel mentioned this pull request Feb 2, 2020

ddbg no longer exists, website dead #10751

Merged

MoonlightSentinel force-pushed the depr-template branch from 2a1510e to 41861ed Compare February 27, 2020 01:29

MoonlightSentinel force-pushed the depr-template branch 4 times, most recently from 1472a83 to 1ecb121 Compare September 3, 2020 15:16

MoonlightSentinel force-pushed the depr-template branch from 1ecb121 to 4669699 Compare September 21, 2020 12:15

dlang-bot added Needs Rebase Needs Work stalled labels May 20, 2021

MoonlightSentinel force-pushed the depr-template branch from 4669699 to 9cd6d44 Compare November 18, 2021 22:41

dlang-bot removed Needs Work Needs Rebase stalled labels Nov 18, 2021

dlang-bot added the Bug Fix label Nov 18, 2021

MoonlightSentinel changed the title ~~Fix Issue 7619 - Broken deprecated feature with template function~~ Fix Issue 7619 - Infer deprecated for template instances Nov 18, 2021

dlang-bot added the stalled label Feb 17, 2022

dlang-bot removed the stalled label Mar 25, 2022

dlang-bot added the stalled label Jun 23, 2022

dlang-bot added Needs Rebase Needs Work labels Jul 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 7619 - Infer deprecated for template instances #10677

Fix Issue 7619 - Infer deprecated for template instances #10677

MoonlightSentinel commented Dec 18, 2019 •

edited

Loading

MoonlightSentinel commented Jan 3, 2020

MoonlightSentinel commented Jan 11, 2020 •

edited

Loading

Geod24 commented Jan 12, 2020

Geod24 commented Jan 12, 2020

MoonlightSentinel commented Jan 13, 2020 •

edited

Loading

Geod24 commented Jan 13, 2020 •

edited

Loading

MoonlightSentinel commented Jan 13, 2020 •

edited

Loading

Geod24 commented Jan 13, 2020

MoonlightSentinel commented Jan 13, 2020

Geod24 commented Jan 13, 2020

MoonlightSentinel commented Jan 13, 2020

MoonlightSentinel commented Jan 14, 2020

Geod24 Jan 19, 2020

MoonlightSentinel Jan 19, 2020

Geod24 Jan 19, 2020

MoonlightSentinel commented Jan 19, 2020

12345swordy commented Jun 11, 2020

MoonlightSentinel commented Jun 11, 2020

dlang-bot commented Nov 18, 2021

RazvanN7 commented Mar 25, 2022

Fix Issue 7619 - Infer deprecated for template instances #10677

Are you sure you want to change the base?

Fix Issue 7619 - Infer deprecated for template instances #10677

Conversation

MoonlightSentinel commented Dec 18, 2019 • edited Loading

MoonlightSentinel commented Jan 3, 2020

MoonlightSentinel commented Jan 11, 2020 • edited Loading

Geod24 commented Jan 12, 2020

Geod24 commented Jan 12, 2020

MoonlightSentinel commented Jan 13, 2020 • edited Loading

Geod24 commented Jan 13, 2020 • edited Loading

MoonlightSentinel commented Jan 13, 2020 • edited Loading

Geod24 commented Jan 13, 2020

MoonlightSentinel commented Jan 13, 2020

Geod24 commented Jan 13, 2020

MoonlightSentinel commented Jan 13, 2020

MoonlightSentinel commented Jan 14, 2020

Geod24 Jan 19, 2020

Choose a reason for hiding this comment

MoonlightSentinel Jan 19, 2020

Choose a reason for hiding this comment

Geod24 Jan 19, 2020

Choose a reason for hiding this comment

MoonlightSentinel commented Jan 19, 2020

12345swordy commented Jun 11, 2020

MoonlightSentinel commented Jun 11, 2020

dlang-bot commented Nov 18, 2021

Bugzilla references

Testing this PR locally

RazvanN7 commented Mar 25, 2022

MoonlightSentinel commented Dec 18, 2019 •

edited

Loading

MoonlightSentinel commented Jan 11, 2020 •

edited

Loading

MoonlightSentinel commented Jan 13, 2020 •

edited

Loading

Geod24 commented Jan 13, 2020 •

edited

Loading

MoonlightSentinel commented Jan 13, 2020 •

edited

Loading