Fix Issue 16206 - traits getOverloads fails when one of the overload is a templatized function #8195

Biotronic · 2018-04-19T20:20:29Z

This adds templates to the result of __traits(getOverloads), allowing them to be iterated and inspected.

There's a few issues other than 16206 that will be fixed by this - I'll have a look-see through bugzilla tomorrow to find them.

It's possible this breaks code by considering template overloads where only function overloads were expected earlier. I expect this to happen rarely in real code, but it's certainly possible. It might be that some templates in Phobos will require updating to handle mixed function/template overload sets.

dlang-bot · 2018-04-19T20:20:32Z

Thanks for your pull request and interest in making D better, @Biotronic! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
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
✓	16206	normal	traits getOverloads fails when one of the overload is a templatized function

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8195"

jacob-carlborg · 2018-04-20T05:53:54Z

It would be nice if the commit message contained the same info as the PR description.

Perhaps worth a changelog entry.

JinShil · 2018-04-20T06:57:47Z

@bbasile Can you please investigate the iz failure in Jenkins?

ghost · 2018-04-20T07:29:40Z

@JinShil, iz fix + tag pushed. Not sure of what was the last change in this PR, so maybe it wasn't necessary to do anything from my side ?
@Biotronic, if you can relaunch the project tester, should be good now.

ghost · 2018-04-20T07:34:06Z

I don't know if adding the templates to the overloads is a good idea. The problem was mostly that the count was different. The fix, as done right now, is actually a breaking change.

Biotronic · 2018-04-20T09:16:37Z

So I was proven wrong immediately:

I expect this to happen rarely in real code

The other option would be to create a new trait, something like getOverloadsIncludingTemplates (or some even better name :p). getTemplateOverloads would also be possible, but it seems likely that users will often be interested in not only templates but all symbols in an overload set.

For that matter, the issue of getOverloads returning nothing when the lexically first overload is a template, is just one part of this fix, and could perfectly well be done without introducing templates in the result.

RazvanN7 · 2018-04-20T09:17:43Z

@Biotronic I worked on this PR a couple of months ago : https://github.com/dlang/dmd/pull/7959/files . Maybe it will help you if you plan on fixing traits(getOverloads) entirely

Biotronic · 2018-04-20T09:21:06Z

I seen it, but considered it separate, since it addresses a different issue. It doesn't seem to touch any of the same code.

ghost · 2018-04-20T09:28:36Z

For that matter, the issue of getOverloads returning nothing when the lexically first overload is a template, is just one part of this fix, and could perfectly well be done without introducing templates in the result.

Yes. This in a first time sounds more reasonable.

ghost · 2018-04-20T09:42:48Z

Possible trick to prevent the breaking change: add an optional bool parameter (which would mean "include templatized funcs or not").

RazvanN7 · 2018-04-20T09:41:38Z

src/dmd/traits.d

+            {
+                VarExp ve = cast(VarExp)ex;
+                auto td = ve.var.isTemplateDeclaration();
+                f = td;


f = ve.var.isTemplateDeclaration and you can get rid of td

No can do - I use td.funcroot later, and that's defined in TemplateDeclaration, not in Dsymbol.

RazvanN7 · 2018-04-20T09:56:43Z

src/dmd/traits.d

+                if (td && td.funcroot)
+                    f = td.funcroot;
+                ex = null;
+            }

            overloadApply(f, (Dsymbol s)
            {
                auto fd = s.isFuncDeclaration();
                if (!fd)


I think the code would look nicer like this:

auto fd = s.isFuncDeclaration(); auto td = s.isTemplateDeclaration(); if (!fd && !td) return 0; if (td) { /* do template stuff */} else { /* do function stuff */} // fd and td are mutually exclusive

RazvanN7 · 2018-04-20T09:58:07Z

test/compilable/Test16206.d

+
+static assert(S.foo() == 0);
+static assert(S.foo(1) == 1);
+static assert(S.foo("") == 2);


add newline

Biotronic · 2018-04-20T11:33:05Z

Removed templates from result, since it makes Jenkins sad. Will make a separate PR for template overloads.

ghost · 2018-04-20T11:40:08Z

Removed templates from result, since it makes Jenkins sad. Will make a separate PR for template overloads.

You misunderstand the problem. The problem is that you didn't only fix the issue, you also broke the semantic of getOverloads. Your separate PR will also make Jenkins sad. This is why i reverted https://github.com/BBasile/iz/commit/6cacdec694aef27e20ef98dcbd235d16f056d2df.

Biotronic · 2018-04-20T11:45:48Z

I'm still somewhat confused. My second PR will be on the form __traits(getAllOverloads), and will not change the semantics of getOverloads at all. Are you saying that having getOverloads return templates is OK?
I filed a bug to explain the suggested semantics. Can't get any work done on it for a few hours, sadly:
https://issues.dlang.org/show_bug.cgi?id=18785

Strictly speaking, there could be code out there that depends on the current behavior of getOverloads, and that will break once it gives proper results when the first element is a template. If we are to worry about that, I'm not sure what to say.

ghost · 2018-04-20T12:18:25Z

I'm still somewhat confused. My second PR will be on the form __traits(getAllOverloads), and will not change the semantics of getOverloads at all. Are you saying that having getOverloads return templates is OK?

Yes but ONLY with a second optional parameter of type bool, so that when the second trait parameter is not specified the current behavior is maintained, fixing the bug and not breaking the world.
Then in std.traits new traits could be added with explicit second parameter.

Biotronic · 2018-04-21T13:27:29Z

Seems sensible. As shown in Test16206.d, __traits(getOverloads) now takes an optional third argument. If this evaluates to true, all overloads are returned - no matter what sort of construct it is. If this evaluates to false, or is not provided, all regular function overloads will be returned. This still fixes the issue of order-of-definition.

ghost

With the hope that the optional param was not a bad idea.
Once something optional is added it's locked forever.

JinShil · 2018-04-21T15:39:30Z

src/dmd/traits.d

+            b = b.ctfeInterpret();
+            if (!b.type.equals(Type.tbool))
+            {
+                e.error("bool expected as third argument of __traits(getOverloads).");


tick marks around bool and __traits(getOverloads) for highlighting in the error message.

Also, we need a test case for this error message.

JinShil · 2018-04-21T15:42:34Z

src/dmd/traits.d

@@ -824,7 +824,7 @@ extern (C++) Expression semanticTraits(TraitsExp e, Scope* sc)
        e.ident == Id.getVirtualMethods ||
        e.ident == Id.getVirtualFunctions)
    {
-        if (dim != 2)
+        if (dim != 2 && !(dim == 3 && e.ident == Id.getOverloads))
            return dimError(2);


codecov shows this line isn't covered in the test suit.

Question: Should this be covered by a separate test file in the fail_compilation folder, or is a static assert(!__traits(compiles)) in Test16206.d good enough?

Scratch that. Looking at the existing tests, I was able to notice a pattern. :p

JinShil · 2018-04-21T15:44:59Z

This will require a documentation update at https://dlang.org/spec/traits.html#getOverloads

Biotronic · 2018-04-21T20:11:58Z

Added changelog entry, and a spec PR in dlang/dlang.org#2351

aliak00 · 2018-04-23T22:41:31Z

Is adding an unnamed boolean argument preferred over having a new trait such as ‘getTemplateOverloads’. Personally I think at the call site the API without the “magic” bool would be quite a bit more readable.

jacob-carlborg · 2018-04-24T06:26:05Z

Since __traits is kind of like a magic function, is it possible to support the following syntax __traits(getOverloads, S, "foo", includeTemplates) without includeTemplates having to be defined?

Biotronic · 2018-04-29T18:54:07Z

Sorry for taking so long to look into this. Of course it's possible - it's basically what __traits(compiles) does. However, it would require much greater changes and make the code harder to follow.

I'm much more partial to aliak00's suggestion of getTemplateOverloads, though I feel that name is somewhat misleading, since we want it to return non-template overloads too. I argue that the best behavior is to return every element of what the compiler considers the 'overload set', which may be a mix of templates and functions, a set of templates, or one of a number of different things. getAllOverloads is a better match, but feels a lot like calling it getOverloads2. 😛 Maybe getOverloadSet? Though that begets the question of how it differs from getOverloads. Ah well.

I've already written and tested that code, so changing it to getAllOverloads would be easy.

JinShil · 2018-05-07T00:59:22Z

@WalterBright, any concerns about this?

aliak00 · 2018-05-07T08:09:49Z

getTemplateOverloads would be misleading if it returned the whole set yeah (while typing I was thinking it would only return template overloads but i failed to type out my entire thought :p).

getOverloadsEx ? ala MS APIs 😱

But all suggestions IMO so far are better than a bool at least.

Biotronic · 2018-05-07T09:05:20Z

getSymbolOverloads?

jacob-carlborg · 2018-05-07T09:13:04Z

getSymbolOverloads?

I think Symbols is too generic. There are other kind of symbols that cannot be overloaded.

Biotronic · 2018-05-07T09:20:08Z

True, but the current implementation will return any symbol, regardless of whether it's possible to overload:

struct S {}
static assert(is(__traits(getOverloads, mixin(__MODULE__), "S", true)[0]));

The reasoning being you often don't really care if it's possible to overload, only that it exists at all.

RazvanN7 · 2018-05-24T10:55:04Z

@JinShil anything else blocking the merge of this PR?

JinShil · 2018-05-24T11:01:28Z

I was hoping @WalterBright or @andralex would weigh in, since it affects the public API.

JinShil · 2018-05-24T11:11:50Z

I was also thinking about maybe using a bitArray enum for the selection criteria.

enum OverloadSelection
{
    nonTemplates = 1;
    templates = 2;
    couldThereBeOthers = 4;
}

__traits(getOverloads, mixin(__MODULE__), "S", OverloadSelection.Templates);

But that may be totally unnecessary. Thoughts?

RazvanN7 · 2018-05-24T19:04:32Z

@JinShil I think that what you propose may be a candidate for std.traits . For now, I think that this PR is ready to go.

jacob-carlborg · 2018-05-29T05:59:09Z

test/fail_compilation/fail16206b.d

+---
+*/
+
+struct S {


Curly braces on their own lines. Applies to all test files and the changelog entry.

OT:
Btw I am not sure whether we need to be so picky for the test files, because it mostly doesn't matter there.
No one looks actively at the tests anyhow, but only when they fail.

That's why I made a comment and not "Request Changes".

BTW, I think that tests should be treated just as the rest of the code. Same standards, style, refactoring and so on.

dlang-bot added the Severity:Bug Fix label Apr 19, 2018

jacob-carlborg approved these changes Apr 20, 2018

View reviewed changes

Biotronic force-pushed the Issue16206 branch from ed258c5 to cac247b Compare April 20, 2018 07:14

RazvanN7 changed the title ~~Fix issue 16206~~ Fix Issue 16206 - traits getOverloads fails when one of the overload is a templatized function Apr 20, 2018

RazvanN7 requested changes Apr 20, 2018

View reviewed changes

Biotronic force-pushed the Issue16206 branch 2 times, most recently from a50b505 to 574e355 Compare April 20, 2018 11:05

Biotronic force-pushed the Issue16206 branch from 574e355 to 7800e8c Compare April 21, 2018 13:21

ghost approved these changes Apr 21, 2018

View reviewed changes

JinShil suggested changes Apr 21, 2018

View reviewed changes

JinShil added Review:Needs Approval Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org and removed Review:Needs Approval labels Apr 21, 2018

Biotronic force-pushed the Issue16206 branch 3 times, most recently from c88bee8 to 9620131 Compare April 21, 2018 19:56

Biotronic mentioned this pull request Apr 21, 2018

Fix Issue 16206 - traits getOverloads fails when one of the overload is a templatized function dlang/dlang.org#2351

Merged

Biotronic force-pushed the Issue16206 branch from 9620131 to 1613686 Compare April 21, 2018 20:14

Fix issue 16206 - getOverloads fails when first overload is template

b52561f

Biotronic force-pushed the Issue16206 branch from 1613686 to b52561f Compare April 21, 2018 20:31

JinShil removed Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels Apr 21, 2018

RazvanN7 approved these changes Apr 22, 2018

View reviewed changes

JinShil added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label May 26, 2018

JinShil approved these changes May 29, 2018

View reviewed changes

jacob-carlborg reviewed May 29, 2018

View reviewed changes

RazvanN7 added Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels May 30, 2018

dlang-bot merged commit 1ea020f into dlang:master May 30, 2018

rmanthorpe mentioned this pull request Jun 18, 2020

Fix Issue 20833 - Template members prevent __traits(getOverloads) from working properly #11295

Merged

Fix Issue 16206 - traits getOverloads fails when one of the overload is a templatized function #8195

Fix Issue 16206 - traits getOverloads fails when one of the overload is a templatized function #8195

Conversation

Biotronic commented Apr 19, 2018 • edited Loading

dlang-bot commented Apr 19, 2018

Bugzilla references

Testing this PR locally

jacob-carlborg commented Apr 20, 2018

JinShil commented Apr 20, 2018

ghost commented Apr 20, 2018 • edited by ghost Loading

ghost commented Apr 20, 2018

Biotronic commented Apr 20, 2018

RazvanN7 commented Apr 20, 2018

Biotronic commented Apr 20, 2018

ghost commented Apr 20, 2018

ghost commented Apr 20, 2018 • edited by ghost Loading

RazvanN7 Apr 20, 2018 • edited Loading

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

Biotronic commented Apr 20, 2018

ghost commented Apr 20, 2018

Biotronic commented Apr 20, 2018 • edited Loading

ghost commented Apr 20, 2018 • edited by ghost Loading

Biotronic commented Apr 21, 2018

ghost 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

Choose a reason for hiding this comment

JinShil commented Apr 21, 2018

Biotronic commented Apr 21, 2018

aliak00 commented Apr 23, 2018

jacob-carlborg commented Apr 24, 2018

Biotronic commented Apr 29, 2018

JinShil commented May 7, 2018

aliak00 commented May 7, 2018

Biotronic commented May 7, 2018

jacob-carlborg commented May 7, 2018

Biotronic commented May 7, 2018

RazvanN7 commented May 24, 2018

JinShil commented May 24, 2018

JinShil commented May 24, 2018

RazvanN7 commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob-carlborg May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Biotronic commented Apr 19, 2018 •

edited

Loading

ghost commented Apr 20, 2018 •

edited by ghost

Loading

ghost commented Apr 20, 2018 •

edited by ghost

Loading

RazvanN7 Apr 20, 2018 •

edited

Loading

Biotronic commented Apr 20, 2018 •

edited

Loading

ghost commented Apr 20, 2018 •

edited by ghost

Loading

jacob-carlborg May 29, 2018 •

edited

Loading