After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 641418 - override keyword not needed when implementing an abstract property declared in an interface?
override keyword not needed when implementing an abstract property declared i...
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Vala maintainers
Vala maintainers
Depends on: 615830 772149 772381
Blocks:
 
 
Reported: 2011-02-04 00:14 UTC by Raul Gutierrez Segales
Modified: 2016-10-06 11:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (2.29 KB, patch)
2016-09-28 04:09 UTC, Matthias Berndt
none Details | Review
vala: Make DataType.equals() to check the type-arguments (2.31 KB, patch)
2016-09-28 07:55 UTC, Rico Tzschichholz
none Details | Review
revert Jürg's change that causes error messages (1.05 KB, patch)
2016-09-29 20:14 UTC, Matthias Berndt
none Details | Review
patch (3.47 KB, patch)
2016-09-30 08:38 UTC, Matthias Berndt
none Details | Review
vala: Check type arguments in DataType.equals (3.63 KB, patch)
2016-10-05 15:55 UTC, Rico Tzschichholz
none Details | Review
vala: Check type arguments in DataType.equals (2.79 KB, patch)
2016-10-06 07:34 UTC, Rico Tzschichholz
committed Details | Review
Do not report internal error for invalid code with nested generics (1.30 KB, patch)
2016-10-06 09:18 UTC, Jürg Billeter
committed Details | Review

Description Raul Gutierrez Segales 2011-02-04 00:14:04 UTC
Why is it that one *doesn't* have to use override when implementing a
property declared on an interface? I.e.:

interface Animal 
{ 
 public abstract GenericArray<string> names_of_bones { get; set; }
}

class Dog : Object, Animal 
{
 public GenericArray<string> names_of_bones { get; set; }  
}                                                                                                
 
works.

If I try:

 public override GenericArray<string> names_of_bones { get; set; }

valac complains with "no suitable property found to override".

And the problem is that this also works (!):

interface Animal 
{ 
 public abstract GenericArray<string> names_of_bones { get; set; }
}

class Dog : Object, Animal 
{
 public GenericArray<int> names_of_bones { get; set; }  
}                                                                                                

which leads to serious bugs. 

Am I missing something?
Comment 1 Luca Bruno 2011-02-11 08:27:16 UTC
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.
Comment 2 Raul Gutierrez Segales 2011-02-11 11:50:14 UTC
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.
Comment 3 Matthias Berndt 2016-09-28 04:09:28 UTC
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 :-)
Comment 4 Rico Tzschichholz 2016-09-28 07:55:08 UTC
Created attachment 336417 [details] [review]
vala: Make DataType.equals() to check the type-arguments
Comment 5 Jürg Billeter 2016-09-28 07:56:08 UTC
More libgee issues:

http://paldo.org:8010/builders/vala-staging/builds/101/steps/libgee/logs/stdio
Comment 6 Rico Tzschichholz 2016-09-28 07:59:28 UTC
Cleaned up and attached the updated patch.

What about the DataType.stricter()?
Comment 7 Matthias Berndt 2016-09-28 14:22:03 UTC
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
Comment 8 Matthias Berndt 2016-09-28 23:27:03 UTC
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.
Comment 9 Matthias Berndt 2016-09-28 23:51:47 UTC
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...
Comment 10 Matthias Berndt 2016-09-29 20:14:35 UTC
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.
Comment 11 Jürg Billeter 2016-09-30 06:28:09 UTC
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
Comment 12 Jürg Billeter 2016-09-30 08:19:03 UTC
I meant, libgee master builds fine with your patch without having to revert the TypeParameter.equals change.
Comment 13 Matthias Berndt 2016-09-30 08:27:12 UTC
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?
Comment 14 Matthias Berndt 2016-09-30 08:38:09 UTC
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…
Comment 15 Rico Tzschichholz 2016-09-30 11:28:35 UTC
Another build with the new patch:
http://paldo.org:8010/builders/vala-staging/builds/103
Comment 16 Matthias Berndt 2016-09-30 22:48:51 UTC
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.
Comment 17 Matthias Berndt 2016-09-30 23:00:15 UTC
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!
Comment 18 Rico Tzschichholz 2016-10-01 17:21:55 UTC
@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
Comment 19 Rico Tzschichholz 2016-10-01 20:00:40 UTC
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.
Comment 20 Matthias Berndt 2016-10-03 21:18:06 UTC
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 :-)
Comment 21 Matthias Berndt 2016-10-05 12:42:28 UTC
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?
Comment 22 Rico Tzschichholz 2016-10-05 13:16:22 UTC
@matthias: You didn't address my fix in Comment 19, or do you expect me tweaking the patch myself again?
Comment 23 Rico Tzschichholz 2016-10-05 15:55:57 UTC
Created attachment 336997 [details] [review]
vala: Check type arguments in DataType.equals

This also reverts "Report internal error for invalid type parameter
comparison" b0191489cb87d15b7c97bb82af2269de6c80fadb
Comment 24 Jürg Billeter 2016-10-05 16:11:00 UTC
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.
Comment 25 Matthias Berndt 2016-10-05 18:59:31 UTC
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.
Comment 26 Matthias Berndt 2016-10-05 19:02:18 UTC
Hey Rico, thanks for taking the time to do it yourself :-)
Comment 27 Jürg Billeter 2016-10-06 01:30:20 UTC
(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.
Comment 28 Matthias Berndt 2016-10-06 06:57:17 UTC
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?
Comment 29 Jürg Billeter 2016-10-06 07:14:21 UTC
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.
Comment 30 Rico Tzschichholz 2016-10-06 07:30:50 UTC
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.
Comment 31 Rico Tzschichholz 2016-10-06 07:34:36 UTC
Created attachment 337044 [details] [review]
vala: Check type arguments in DataType.equals
Comment 32 Jürg Billeter 2016-10-06 07:52:16 UTC
Review of attachment 337044 [details] [review]:

Sure, please go ahead.
Comment 33 Matthias Berndt 2016-10-06 07:57:02 UTC
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.
Comment 34 Rico Tzschichholz 2016-10-06 07:59:11 UTC
Attachment 337044 [details] pushed as 2bf3d97 - vala: Check type arguments in DataType.equals
Comment 35 Matthias Berndt 2016-10-06 08:06:53 UTC
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...
Comment 36 Jürg Billeter 2016-10-06 08:14:19 UTC
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).
Comment 37 Matthias Berndt 2016-10-06 08:39:15 UTC
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.
Comment 38 Jürg Billeter 2016-10-06 09:18:57 UTC
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?
Comment 39 Matthias Berndt 2016-10-06 10:37:24 UTC
Yes, that looks good.
Comment 40 Jürg Billeter 2016-10-06 11:10:50 UTC
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>