-
-
Notifications
You must be signed in to change notification settings - Fork 608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue 16486: Alias template function parameter resolution #9778
Conversation
|
Thanks for your pull request and interest in making D better, @baziotis! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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
Testing this PR locallyIf 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#9778" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks great!
| //printf("prmtype: %s\n", prmtype.toChars()); | ||
| while (prmtype.ty == Tinstance) | ||
| { | ||
| TypeInstance prmti = cast(TypeInstance) prmtype; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can probably do while (TypeInstance prmti = prmtype.isInstance()) (if isInstance() doesn't exist please create it) similar to the if (auto td = prmti.tempinst.tempdecl.isTemplateDeclaration()) below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is the isTemplateInstance but that is for symbols. If I understand it correctly, code will look kind of weird if I introduce that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isTypeInstance()?
Line 2663 in 9585ab8
| inout(TypeInstance) isTypeInstance() { return ty == Tinstance ? cast(typeof(return))this : null; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I needed, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thats what it is. Sorry about that, I'm going to blame travelling and jet lag for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't mention it. It's odd though, because I saw the idiom I used in a lot of places in the compiler and not once the isTypeInstance(). It might be a recent addition...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still want that? Because I think it can't happen because it is a while and not an if.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes:
if (TypeInstance prmti = prmtype. isTypeInstance())
{
Objects* tempargs = prmti.tempinst.tiargs;
while ((prmti = prmtype. isTypeInstance()))
{
//...
}
}
|
Thanks Nicholas for the review, but unfortunately those are minor details compared to the current problem. I didn't have much time today, but I'll try again tomorrow. Here are some problems in case anyone has any suggestion. The one problem is that we need to handle cyclic template declarations. One example of that is But that's not the most important problem. This |
…nline to find the declaration
|
I want to stress that I did not pay much attention to the quality of the code.
|
Please do rebases instead as this will keep the commit history clean.
We're aware, but the better habit is to things correctly from the start ;-) |
Good to know.
Of course, It's not that I didn't take the time to make it better out of boredom, I will happily do it. But I thought that there's no point to restructure it if the approaches used are not desirable.
I should have used the draft PRs for the previous commits indeed, I didn't know it. But at this point, I believe it is the time for someone to look closer. |
| @@ -0,0 +1,36 @@ | |||
| // Example 1 - Simple case | |||
| struct TestType(T, Q) { } | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following doesn't work.
| struct TestType(T, Q) { } | |
| struct TestType(A, B) { } | |
| alias TestAliasA(A, B) = TestType!(A, B); | |
| alias TestAlias(T, Q) = TestAliasA!(T, Q); |
Maybe you can replace the identifiers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither does this.
| struct TestType(T, Q) { } | |
| struct TestType(T, Q) { } | |
| template TestAliasB(T, Q) | |
| { | |
| alias A = T; | |
| alias TestAliasB = TestType!(A, Q); | |
| } | |
| alias TestAlias(T, Q) = TestAliasB!(T, Q); |
alias A = T should be resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second is still a little obscure to me. I'll try to understand it again.
For the first one, I understand why it makes sense to work. But, trying to implement it, here are the consequences. At some point, we reach the declaration TestAliasA(A, B) and this has type TestType!(A, B). This I think is like an incomplete type. Incomplete because of the A, B, which are identifiers, and their resolution depends on the scope. This is resolved depending on where it's used.
It seems I could replace A, B with whatever were the initial parameters. The problem with that is that we have now changed the type of the declaration TestAliasA. Code-wise, the consequence of that is that if we reach a template and its argument names don't match exactly the initial ones, we have to create a new type. I think that no matter how many alias indirections we want, we can just make a copy only in the last (the one that resolves to a non-alias template instance).
Is that acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it seems that both pass. That was too easy to be correct though...
Just a note: It makes sense only if the num of root parameters and final parameters is the same. Not that it is difficult to take it into account, but if they're not the same, there is a more difficult problem anyway. The one on the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too simplistic. This doesn't wrok:
| struct TestType(T, Q) { } | |
| alias TestAlias(T, Q) = TestAliasA!(Q, T); |
You should follow the parameters throughout, don't count on the order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I realized, after the commit, by testing something like this, that some "following cleverness" should be added. Thanks though! Actually, for such parameters it is not difficult. But I didn't update it since then I thought about cases like:
struct TestType(A, B) { }
alias TestAlias(A: A*, B) = TestType!(A*, B);
alias TestAlias2(T, Q) = TestAlias!(T*, Q);
Something like that. Here, A* is different from T (the initial argument) and so it will be replaced by T.
Meaning, parameters are not just identifiers. Still, it seems feasible because the ways that you can pass template parameters, however complicated they might be, all "look" like a declaration (they may even be parsed like that, I haven't looked). That is, they have only one identifier and that is the only you need to find and replace. But I better look at it tomorrow with a clear head.
Lastly, the follow-through, as said before, at least as I'm thinking it now, requires copies all the way.
| // and 'b' goes back to 'a'. For now, we save the previous type | ||
| // and test it against (if they're equal) the new type. | ||
| // TODO: In the general | ||
| // case, we probably need full DFS cycle detection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One common "trick/hack/easy solution" in other parts of DMD is to use an iteration counter with a cut-off (e.g. 1000).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, those can come in handy. It also seems that the problem does not require full cycle detection because there's always only one way to go, meaning it's like floyd's tortoise and hare. I don't know if this is going to be useful though..
|
This needs a DIP to clearly spell out how the changes to template matching / instantiation. |
| Type savetype = prmtype; | ||
| Type prevtype = null; | ||
| //printf("prmtype: %s\n", prmtype.toChars()); | ||
| if(prmtype.ty == Tinstance) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ on new line.
Thank you, it's in progress, still on draft version. |
|
I'm postponing this PR for now. Your feedback, a discussion with one of my professors and me |
|
Here is a first somewhat complete try on the DIP: https://github.com/baziotis/DIPs/blob/master/Drafts/1NNN-SB.md Anyone's feedback is greatly appreciated. @WalterBright and @andralex your input is especially valuable. |
|
@baziotis The DIP link is broken. |
|
Fixed thanks! |
|
In the example motivating the DIP: auto foo(T)(Slice!(StairsIterator!(T*, "-")) m)
{
}
alias PackedUpperTriangularMatrix(T) = Slice!(StairsIterator!(T*, "-"));
// fails, issue 16486
auto foo(T)(PackedUpperTriangularMatrix!T m) { }Why wouldn't this be sufficient? enum isPackedUpperTriangularMatrix(T) = is(T: Slice!(StairsIterator!(U*, "-")), U);
auto foo(T)(T m) if(isPackedUpperTriangularMatrix!T) { /* ... */ } |
Because it is much more complex for real-world API. void composeLU(T)(
PackedLowerTriangularMatrix!(const T) l, // First Slice type for Lower Matrix
PackedUpperTriangularMatrix!(const T) u, // Second Slice type for Lower Matrix
BlasMatrix!T lu, // Third Slice type for General Matrix
)
if (is(T == float) && is(T == double))
{
...
}A workaround without resolution looks ugly, requires much more code, and very unreadable. |
|
I don't see how this is so much more complicated: void composeLU(L, U, LU)(L l, U u, LU lu)
if(IsPackedLowerTriangularMatrix!L
&& isPackedUpperTriangularMatrix!U
&& isBlasMatrix!LU
&& /* .. */
)
{
} |
The devil in the details, the EDIT: plus they skipped conditions check that the floating point type is constrained to EDIT2: in addition, the user code very likely needs to get this floating point type to perform other operations. So, the user needs to add a further statement in the function body. |
|
and then consider the error message you get if that goes ever so slightly wrong (for both users trying to write such constraints and for users (mis)using such a function). |
Yes, and the devil is similarly in the details of the guts of the type aliases.
Would it be acceptable to leave out the template constraints? In most cases I see in the wild, templates are over-constrained anyway (especially if checking for a particular type as is the case in the examples here, instead of what the type can actually do). I'm confused at what kind of user would be able to correctly write a type alias but not a template constraint. The DIP seems to me like a weaker version of Rust's traits or Haskell's typeclasses, so why accept a weaker version if the language is going to be changed anyway? |
I wholeheartedly agree that getting better error messages that guide the user should be a goal. However, I see no discussion on how the DIP would ameliorate the current situation. |
No. It is not acceptable for numeric and data libraries in most of the cases. For example, a function can use precompiled D and/or C kernels or call the functions that have stronger constraints.
You didn't write the correct version, it was incomplete. I don't want my libraries to test users abilities to write constraints. I want libraries that help users.
Well, this is the news for me (from dconf?). Is it D3? Who, when, how will change the language? Where the roadmap and new features descriptions can be found?
I don't know about what situation you are writing. Looks like you missed the point I wrote above. |
What would happen if the contraints weren't there?
I focussed on the template contraints that were the subject of the PR and DIP. I wrongfully assumed the rest was implied.
I meant: why write a DIP for a weaker version of traits? Especially since, unless I'm missing something, it's just syntax sugar (and not a whole lot) for what can be done with template constraints currently?
I'm writing about better error messages, since that's what @thewilsonator mentioned. |
It may not compile with a useless error message. Or, it may compile and produce the wrong numeric code. Or, it may break the API completely because other functions overload the same name. The constraints are required because of the same reasons Phobos requires them, for the numeric code they are even more sensible. For simple algorithms like
Because of the complexity of API without this feature. The constraint will look too large.
The end of #9778 (comment):
... maybe except some API inherited from Phobos. |
I don't see much of a difference between large template constraints and large parameter lists. The example given above looks nearly identical to me in either form. |
OK. I don't force you to change your vision. |
|
@atilaneves Thanks for feedback. I agree with Ilya on this discussion. Generally, it seems to me that with this feature, we can have more readable code than the alternatives.
|
Right, currently template aliases decay to the underlying type when doing type checking like all aliases do, however they really should behave more like a type definition for the purposes of matching (a It should behave like #include <vector>
template<typename T>
using vec = std::vector<T>;
void foo(vec<float>) {}
int main(int,char**)
{
vec<float> a;
foo(a);
} |
|
Might have missed something, but as parameters in template functions, they don't "decay to the underlying type". That is the problem. I mean this is the (not good) exception. Yes, my intention was too to work like |
Thats annoying. |
|
Really, that's not specific to |
|
Since a workaround exists with template constraints, I continue to struggle to see the value in this. @WalterBright ? |
|
I think me and @9il explained our opinions. You too, and thanks for the feedback. |
|
@baziotis, @atilaneves is the new @andralex as of https://youtu.be/cpTAtiboIDs?t=3074 |
A rhetorical question: Why Atila, why not Seb? |
|
Perhaps Atila volunteered. As far as I understand he's working for Symmetry now and has some dedicated time to do "Andrei work". |
Congratulations for that Atila! I didn't know it. Again, the same thing is true. If there's no new info from both parties, then we should wait for the standard procedure. But, if Atila is part of the final acceptance and he thinks is not a good idea, then we should hear from @WalterBright and maybe drop it sooner if it is to be dropped anyway? |
Because it is faaaaar easier to write and correctly use types, than constraints. A template signature has three parts: compile time parameters, runtime parameters (which may depend on the compile time ones) and constraints (ditto).
You can write generic code in C, too. Just because you can work around it, doesn't mean there isn't a problem that would benefit from being solved. |
|
This thread has been closed, but it seems to me this issue really needs fixing. If I already have a well-defined I'd even be happy with the constraint solution if it's not so counter-intuitive to implement. struct Matrix(U, size_t M, size_t N)
{
}
alias Vector(U, size_t N) = Matrix!(U, N, 1);
enum isVector(U) = is(U == Vector!(S, N), S, size_t N); // I guess it fails for the same reason.
enum isVectorCorrect(U) = is(U == Matrix!(S, N, 1), S, size_t N); // the "correct" way and how much I like to REPEAT myself!
void foo(U)(Vector!(U, 3) a)
{
}
void bar(U)(U a) if (isVector!U)
{
}
void main()
{
import std.stdio;
Vector!(float, 3) v;
foo(v); // Error: none of the overloads of template `app.doSomething` are callable using argument types `!()(Matrix!(float, 3LU, 1LU))
bar(v); // failed constraint isVector!U
} |
This is a 3-day prototype implementation of this DIP PR: dlang/DIPs#156
More comments / info on the implementation are in this article: http://users.uoa.gr/~sdi1600105/dlang/alias.html
So, if a parameter is a template alias, we iteratively (because a template alias might be alias to another template alias) reach the actual type of the parameter before proceeding into any type-checking.
The main idea is:
It fails (at least) in such cases:
This basically resolves to this:
This code breaks and rightly so, independently of the bug. However, I'm not sure whether the initial code should break.