Thanks for reporting the bug. Simply override keyword is not needed for interfaces, it's a design choice.
What bugs does it lead to? If you don't implement the property you get an error.
Closing as not a bug, please reopen if needed.

As showed in the example code, I don't think that its correct for the compiler *not* to complain when a property has been defined as:

public abstract GenericArray<string>

and you implement it as:

public GenericArray<int> 

The generated C code will lead to runtime crashes.

Created attachment 336414 [details] [review]
patch

I think Jürg already did most of the hard work in dadf2b48d4da3f1afd95991bec4d4179e231b010 , however DataType.equals is broken. This patch fixes that. Raul's code doesn't compile any longer with this fix :-)

Created attachment 336417 [details] [review]
vala: Make DataType.equals() to check the type-arguments

More libgee issues:

http://paldo.org:8010/builders/vala-staging/builds/101/steps/libgee/logs/stdio

Cleaned up and attached the updated patch.

What about the DataType.stricter()?

Hey Jürg,

thanks for pointing that out, I think that's actually a bug in the patch rather than in libgee. I'll take a look.

Cheers,
Matthias

Ok,

so part of the problem is certainly libgee. There is at least one case where a generic type is applied to the wrong number of arguments (which we should check for – given that CCode(simple_generics) exists, I can't think of a reason not to enforce the correct number of parameters).

Then there is at least one case where two type parameters were flipped, similar to the following example (which I intend to add as a test):
class Foo<A, B> {}
class Bar<A> {
  public virtual Foo<A, B> f<B>() { return f(); }
}
class Baz<A> : Bar<A> {
  public override Foo<B, A> f<B>() { return f(); }
}
This also clearly shouldn't compile and is thus a bug in libgee.

However this also exposes a bug in valac, which refuses to compare two type parameters whose parent symbol is not the same. That assertion needs to go or at least change in some way.

That's what I found so far, but there might be other problems with this patch. Anyway, I'll also prepare a patch for libgee to fix the issues I found.

I've reported a bug with a patch for the issues in libgee, bug 772149. 

Other than the above-mentioned bug in TypeParameter.equals libgee doesn't seem to expose any more bugs in this patch. I'll upload a new revision soon...

Created attachment 336542 [details] [review]
revert Jürg's change that causes error messages

I think that Jürg's assumption that only type parameters with the same parent symbol should be compared is simply not correct, so I propose to revert that change.

I can't think of a case where comparing type parameters of different parent symbols makes sense. libgee master builds find with this revert. Why do you think this is still an issue?

Other projects are failing, though:
http://paldo.org:8010/builders/vala-staging/builds/102

I meant, libgee master builds fine with your patch without having to revert the TypeParameter.equals change.

Hey Jürg,

Yes, it holds when you're compiling correct code. But it doesn't when you compile broken code, like my example in comment 8. The compiler shouldn't crash when you throw broken code at it, right?

Created attachment 336617 [details] [review]
patch

Add another test case and merge the two patches into one. Unfortunately I can't obsolete the other one since it was added by Rico…

Another build with the new patch:
http://paldo.org:8010/builders/vala-staging/builds/103

Well, shotwell failed because of an error in the configure script, I don't think that's related to the patch.

The bindings build fails due to a bug in clutter-gst 2.0.
It is correctly declared to return a List of Gst.TagList here:
https://github.com/GNOME/clutter-gst/blob/clutter-gst-2.0/clutter-gst/clutter-gst-video-texture.c#L803
But in the player class it's declared to return a List of utf8:
https://github.com/GNOME/clutter-gst/blob/clutter-gst-2.0/clutter-gst/clutter-gst-player.c#L2465
I'd report/fix it, but I can't find the clutter-gst product in gnome bugzilla :-(

I spent some time chasing the bug in valadoc and getting all crazy because I couldn't reproduce it and then realised that you had already fixed it.

Maybe I'll get around to looking at some of the other bugs tomorrow.

Ok, just noticed that you fixed rygel and boxes as well, so I'll assume you dealt with all the ones in between as well – thanks!

@matthias: Yeah, at least the projects we are tracking are building fine.
Shotwell does too.

See http://paldo.org:8010/builders/vala-staging/builds/105

@juergi: What about the reverted change in 0.34 now? 

https://git.gnome.org/browse/vala/commit/?h=0.34&id=b0191489cb87d15b7c97bb82af2269de6c80fadb

Better make this test only fail on the type mismatch rather than additionally a fault inferface implementation.

--- a/tests/objects/bug641418-1.test
+++ b/tests/objects/bug641418-1.test
@@ -7,7 +7,7 @@ interface Foo
 
 class Bar : Object, Foo
 {
-       public override GenericArray<int> baz () {
+       public GenericArray<int> baz () {
                return null;
        }
 }


Please take a look at tracker which is failing too.

I've taken a look at tracker and submitted a patch, see bug 772381. I'm not sure I've found all instances of the problem since I disabled quite a few features in ./configure, but one can hope :-)

So what's the status here? The patch helped finding quite a few bugs (among them the quite subtle bug 772381 mentioned earlier) and it doesn't seem to cause any false positives. Is there any reason left not to merge it?

@matthias: You didn't address my fix in Comment 19, or do you expect me tweaking the patch myself again?

Created attachment 336997 [details] [review]
vala: Check type arguments in DataType.equals

This also reverts "Report internal error for invalid type parameter
comparison" b0191489cb87d15b7c97bb82af2269de6c80fadb

I still consider it a compiler bug when TypeParameter.equals is called for type parameters with different parent symbols. It makes no sense to perform such comparisons and allowing them hides compiler bugs, see bug 771964 for an example of a previously hidden bug.

The proper fix may not always be easy. However, as long as valid code compiles without issues and there are no crashes with invalid code, I think we should keep it as it is.

Hi Jürg,

the problem is that there *is* a crash with invalid code. I have shown such code in Comment 13. And that's not even the only case, there are others such as when nesting multiple classes with type parameters.

Hey Rico, thanks for taking the time to do it yourself :-)

(In reply to Matthias Berndt from comment #25)
> the problem is that there *is* a crash with invalid code. I have shown such
> code in Comment 13. And that's not even the only case, there are others such
> as when nesting multiple classes with type parameters.

valac does not crash here with that code. It prints the internal error message but it also prints the regular error message and there is no crash or assertion failure.

Yes, prints an internal error, that is a crash to me. Anyway, let's not argue about the definition of crash. Either way, the compiler shouldn't print an internal error message when confronted with code like that, right?

Yes, this is certainly the goal. However, the proper fix is to ensure valac never performs nonsensical comparisons. I prefer not to hide this issue to reduce the risk of forgetting about it. The internal error message may help expose further bugs such as bug 771964.

In contrast to a crash, this internal error message is not a major issue for users, as the proper error message is printed as well. And this mostly cosmetic problem will be here only until the underlying bug is fixed.

I agree this is a non-fatal issue, it might be confusing for users, but it is tagged as "internal error" nonetheless.

I'd like to finally push this without including the revert.

Created attachment 337044 [details] [review]
vala: Check type arguments in DataType.equals

Review of attachment 337044 [details] [review]:

Sure, please go ahead.

Jürg, could you elaborate a bit on why you think such comparisons are nonsensical? Because afaics they are not only sensible but necessary to typecheck programs like the one in comment 13. And I can show other cases of you insist, trivial ones even.

Attachment 337044 [details] pushed as 2bf3d97 - vala: Check type arguments in DataType.equals

So, what are we going to do about the remaining bug, i.e. the internal error message? I suggest discussing it here, but we can also open a new bug...

A type parameter name has no meaning outside the parent symbol that declares it. To compare generic types across different parents, the compiler has to transform the types to have a common reference point (using get_actual_type) and only then can the type parameter name be used for comparison. Consider the following example (equivalent to example in comment 8):

class Foo<A, B> {}

class Bar<C> {
  public virtual Foo<C, D> f<D>() { return f(); }
}

class Baz<E> : Bar<E> {
  public override Foo<F, E> f<F>() { return f(); }
}

To compare the return types of the two methods, Foo<F, E> has to be transformed to Foo<D, C> and then it can be trivially compared (parent symbols now match for both type arguments). Straight comparison of C and F or D and E doesn't provide any value and indicate that the compiler is missing the necessary transformation somewhere.

I haven't had time to look into this in detail. However, it's possible the underlying issue is that when the compiler is unable to properly transform the type, it simply falls back to the untransformed type and performs the comparison anyway. Instead of bailing out early (or possibly using InvalidType instead of the untransformed type).

Hey Jürg,

well, I have looked at it in detail, that's why I know the check doesn't make sense. Foo<F, E> will be transformed to Foo<D, C>. Then Foo<D, C> will be compared to Foo<C, D> and then the internal error message will trigger because C's parent is the class Bar and D's parent is the method Bar.f.

Your check would make sense if symbols with type parameters couldn't be nested, but they can. Not only can a generic method live within a class, generic classes can also be nested.

Created attachment 337049 [details] [review]
Do not report internal error for invalid code with nested generics

You're right, I missed that. The idea was to detect cases where the transformation is missing to uncover compiler bugs. However, the above comparison between C and D obviously makes sense and is necessary.

I've pushed the attached patch to staging. It should no longer report internal errors for valid comparisons while still detecting bad comparisons. Does this look good to you?

Yes, that looks good.

Comment on attachment 337049 [details] [review]
Do not report internal error for invalid code with nested generics

commit 1fcd716d3b9b3d7b740a4c488cd6a28c0e5f0414
Author: Jürg Billeter <j@bitron.ch>
Date:   Thu Oct 6 11:07:59 2016 +0200

    Do not report internal error for invalid code with nested generics
    
    Reported-by: Matthias Berndt <matthias_berndt@gmx.de>