Created attachment 243872 [details] [review]
0001-Check-the-generic-types-while-checking-the-subclasse.patch

Created attachment 243873 [details] [review]
0002-Force-the-exact-genetic-types.patch

Created attachment 243874 [details] [review]
0003-Remove-incorrect-covariant-casting-from-Vala-sourcec.patch

Also see https://bugzilla.gnome.org/show_bug.cgi?id=615830 . Enforcing exact generic types breaks existing code that could be correct to some extent, isn't it?

(In reply to comment #4)
> Also see https://bugzilla.gnome.org/show_bug.cgi?id=615830 . Enforcing exact
> generic types breaks existing code that could be correct to some extent, isn't
> it?

For some definition of correctness - yes. However I would prefer to force the users to fix them by fixing #694895. Just allowing covariance/contravariance in generics makes the type system completely unsound.

(In reply to comment #5)
> For some definition of correctness - yes. However I would prefer to force the
> users to fix them by fixing #694895. Just allowing covariance/contravariance in
> generics makes the type system completely unsound.

Since there's some "correct" usage, it may break valid users.

(In reply to comment #6)
> (In reply to comment #5)
> > For some definition of correctness - yes. However I would prefer to force the
> > users to fix them by fixing #694895. Just allowing covariance/contravariance in
> > generics makes the type system completely unsound.
> 
> Since there's some "correct" usage, it may break valid users.

1. This make the type system unsound. In effect it is quite easy to produce code which returns say Gtk.Window as Gtk.Label (by mistake) - I've run into it a few times and they are hard to track
2. All legal uses can be fixed by bug #694895 (I can add patches for it as well) - they require small, trivial code changes
3. Arguably the broken cases are much less common then the broken cases of bug #645850
4. The comment in code was quite clear that current behavior was unintentional

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > For some definition of correctness - yes. However I would prefer to force the
> > > users to fix them by fixing #694895. Just allowing covariance/contravariance in
> > > generics makes the type system completely unsound.
> > 
> > Since there's some "correct" usage, it may break valid users.
> 
> 1. This make the type system unsound. In effect it is quite easy to produce
> code which returns say Gtk.Window as Gtk.Label (by mistake) - I've run into it
> a few times and they are hard to track
> 2. All legal uses can be fixed by bug #694895 (I can add patches for it as
> well) - they require small, trivial code changes
> 3. Arguably the broken cases are much less common then the broken cases of bug
> #645850
> 4. The comment in code was quite clear that current behavior was unintentional

Sorry - that come out much angrier/harsher then I intended. I see your point about breaking code but IMHO fixing it is worth the small code breakage.

Hmm. Maybe it would be more feasible to move it in steps:

 - Say in 0.22 (or whenever version the patches will land) the warnings are emitted if now illegal cast is performed (or errors if promoted)
 - In 0.24 the errors are emitted unless downgraded to warnings by compiler option
 - In 0.26 it is enforced

Now the problem is that the assignment can be done without a cast in the vala code: Foo<Bar> foo = new Foo<Baz>();

The important thing is that current code can be rewritten in a such way that the warning goes away. Is it still possible to use a cast if a user wants to do a covariant/variant conversion? I.e.:
Foo<Bar> foo = (Foo<Bar>) new Foo<Baz>();

(In reply to comment #10)
> Now the problem is that the assignment can be done without a cast in the vala
> code: Foo<Bar> foo = new Foo<Baz>();
> 

My feeling is that this is preciously the kind of assignments it would be good to get rid of. The 'correct' usage would be:

class MyObject { ... }
class MyObject2 : MyObject { ... }

void doSomething(List<MyObject> list) {
    MyObject o = list[0];
    o.doSomethingElse ();
    // No list.add (new MyObject()); or similar things
}

List<MyObject2> l = ...;
doSomething(l); // Cast from List<MyObject2> to List<MyObject>

Which should be converted into (not covered by those patches but I would prefer it that way):

void doSomething(List<? : MyObject> list) {
    MyObject o = list[0]; // Ok ? : MyObject can be cast to MyObject
    o.doSomethingElse ();
    // No list.add (new MyObject()); or similar things as MyObject cannot be cast to ? : MyObject
}

Or (that one changes the ABI and have slightly different behaviour):

void doSomething<A : MyObject>(List<A> list) {
    MyObject o = list[0]; // Ok A extends MyObject can be cast to MyObject
    o.doSomethingElse ();
    typeof(A);
    // No list.add (new MyObject()); or similar things as MyObject cannot be cast to ? extends MyObject
}

> The important thing is that current code can be rewritten in a such way that
> the warning goes away.

If getting rid of implicit covariance/contravariance is a goal I'm not sure that suppressing warnings is a good thing. I don't think there is a way to get rid of warnings on copying owned delegates.

It might be a good thing to have a global system of suppressing warnings (per method or global):

[Warning (ignore = "copying-delegate,unsafe-cast")]
void foo(owned VoidFunc f, int i) {
    if (i >= 2) {
        foo (f, i - 1);
        foo (f, i - 1); // Warnings suppressed
    } else {
       Foo<Bar> foo = (Foo<Bar>)new Foo<Baz>(); // Warnings suppressed
    }
}

This might be a separate goal (currently libgee have tons of warnings about unused variables which can be safely ignored).

> Is it still possible to use a cast if a user wants to do
> a covariant/variant conversion? I.e.:
> Foo<Bar> foo = (Foo<Bar>) new Foo<Baz>();

I would much prefer to have Vala put to proper covariance/contravariance as a goal. If Baz extends Bar the such conversion would be legal:

// Ok. Baz extends Bar
Foo<? : Bar> foo = new Foo<Baz>();
// Ok. explicit cast and Baz might be ? extends Bar
Foo<Baz> bar = (Foo<Baz>)foo;
// This should be an error eventually
Foo<Bar> foo2 = (Foo<Bar>)new Foo<Baz>();
// Ok. Baz extends Bar
Foo<Baz : ?> bar2 = new Foo<Bar>();

I haven't implement it yet but that was going to be my next step. (PS. ':' is just an example  - Scala uses + and - while Java super and extends etc.).

(In reply to comment #11)
> (In reply to comment #10)
> > Now the problem is that the assignment can be done without a cast in the vala
> > code: Foo<Bar> foo = new Foo<Baz>();
> > 
> 
> My feeling is that this is preciously the kind of assignments it would be good
> to get rid of. The 'correct' usage would be:
> 
> class MyObject { ... }
> class MyObject2 : MyObject { ... }
> 
> void doSomething(List<MyObject> list) {
>     MyObject o = list[0];
>     o.doSomethingElse ();
>     // No list.add (new MyObject()); or similar things
> }
> 
> List<MyObject2> l = ...;
> doSomething(l); // Cast from List<MyObject2> to List<MyObject>
> 
> Which should be converted into (not covered by those patches but I would prefer
> it that way):
> 
> void doSomething(List<? : MyObject> list) {
>     MyObject o = list[0]; // Ok ? : MyObject can be cast to MyObject
>     o.doSomethingElse ();
>     // No list.add (new MyObject()); or similar things as MyObject cannot be
> cast to ? : MyObject
> }
> 
> Or (that one changes the ABI and have slightly different behaviour):
> 
> void doSomething<A : MyObject>(List<A> list) {
>     MyObject o = list[0]; // Ok A extends MyObject can be cast to MyObject
>     o.doSomethingElse ();
>     typeof(A);
>     // No list.add (new MyObject()); or similar things as MyObject cannot be
> cast to ? extends MyObject
> }
> 
> > The important thing is that current code can be rewritten in a such way that
> > the warning goes away.
> 
> If getting rid of implicit covariance/contravariance is a goal I'm not sure
> that suppressing warnings is a good thing. I don't think there is a way to get
> rid of warnings on copying owned delegates.
> 
> It might be a good thing to have a global system of suppressing warnings (per
> method or global):
> 
> [Warning (ignore = "copying-delegate,unsafe-cast")]
> void foo(owned VoidFunc f, int i) {
>     if (i >= 2) {
>         foo (f, i - 1);
>         foo (f, i - 1); // Warnings suppressed
>     } else {
>        Foo<Bar> foo = (Foo<Bar>)new Foo<Baz>(); // Warnings suppressed
>     }
> }
> 
> This might be a separate goal (currently libgee have tons of warnings about
> unused variables which can be safely ignored).
> 
> > Is it still possible to use a cast if a user wants to do
> > a covariant/variant conversion? I.e.:
> > Foo<Bar> foo = (Foo<Bar>) new Foo<Baz>();
> 
> I would much prefer to have Vala put to proper covariance/contravariance as a
> goal. If Baz extends Bar the such conversion would be legal:
> 
> // Ok. Baz extends Bar
> Foo<? : Bar> foo = new Foo<Baz>();
> // Ok. explicit cast and Baz might be ? extends Bar
> Foo<Baz> bar = (Foo<Baz>)foo;
> // This should be an error eventually
> Foo<Bar> foo2 = (Foo<Bar>)new Foo<Baz>();
> // Ok. Baz extends Bar
> Foo<Baz : ?> bar2 = new Foo<Bar>();
> 
> I haven't implement it yet but that was going to be my next step. (PS. ':' is
> just an example  - Scala uses + and - while Java super and extends etc.).

Current code must keep working. Casts must never be an error. Variance/covariance require much more insight than that. This patch is going nowhere if casts are not allowed to avoid the warning.

(In reply to comment #12)
> Current code must keep working. Casts must never be an error.
> Variance/covariance require much more insight than that. This patch is going
> nowhere if casts are not allowed to avoid the warning.

Wait - are we talking about warnings or errors? Warnings should still keep the code working unless someones compiling with -Werror (but then breakage on change of compiler can be expected).

If the casts would not result in errors (at least for foreseeable future) my guess would be that a generic warning-suppressing mechanism would be better then one-time method as cast.

I understand that current state of patches is not-acceptable but I'm currently argue for:

 - Adding generic warning-suppressing mechanism (bug #700375)
 - Add proper covariance/contravariance notation (bug #694895)
 - Adding warnings when code that worked does not use proper covariance/contravariance notation. Users can still suppress warning by generic mechanism as proposed in bug #700375
 - Some time in future (Vala 1.0?) change the warnings into errors

(In reply to comment #10)
> Foo<Bar> foo = (Foo<Bar>) new Foo<Baz>();

Coming to think about it - in the patches currently there IS a method to cast anything to anything (even without warnings):

Foo<Bar> foo = (Foo)new Foo<Baz>();

My guess is it would be more acceptable as it explicitly wipes the generic information on cast.

*** Bug 701227 has been marked as a duplicate of this bug. ***

Review of attachment 243873 [details] [review]:

.

::: vala/valadatatype.vala
@@ +323,3 @@
 					// the additional check would be very inconvenient, so we
 					// skip the additional check for now
+					if (!type_arg.equals (target_type_arg)) {

I'm fine with this, I'm only concerned about the fact that it will lead to an error rather than a warning. Have to think about that before landing.

::: vala/valagenerictype.vala
@@ +56,3 @@
+			return false;
+		}
+		return gtarget_type.type_parameter.name == type_parameter.name;

I believe checking the name is not enough. What about gtarget_type.type_parameter == type_parameter .

Created attachment 251073 [details] [review]
0001-Consider-the-generics-while-checking-the-validity-of.patch

I've changed it to an warning. Possibly better then return bool and parameter strict would be returning something like?:

enum Vala.Compatibility {
   FULL,
   GENERIC_VIOLATION,
   NONE,
}

Created attachment 251074 [details] [review]
0002-Remove-the-new-warnings-from-Vala-code.patch

Patch removing the warnings. Probably should be updated once bug #694895 will be added.

Review of attachment 251073 [details] [review]:

Thanks. What about compatible (DataType target_type, bool strict = false, SourceReference? reference = false) ?
Also could you explain in detail what the "deprecated implicit conversion" mean if it's not strict, and in which case it happens?

::: vala/valaobjecttype.vala
@@ +69,3 @@
+	}
+
+	public override bool is_subtype_of (DataType ancestor) {

Why did you need this new method?

@@ +70,3 @@
+
+	public override bool is_subtype_of (DataType ancestor) {
+		if (base.is_subtype_of (ancestor)) {

Can base be null here?

(In reply to comment #18)
> Review of attachment 251073 [details] [review]:
> 
> Thanks. What about compatible (DataType target_type, bool strict = false,
> SourceReference? reference = false) ?

That was my first try but the SourceReference? reference was always at the beginning. I don't care either way.

> Also could you explain in detail what the "deprecated implicit conversion" mean

It is suppose to mean that a conversion between generics is made. I.e.

HashSet<HashSet<G>> hs = new HashSey<HashSet<H>>();

> if it's not strict, and in which case it happens?
> 

The compatible is called recursively and for larger assignments I wanted to have only one warning for whole assignment.

> ::: vala/valaobjecttype.vala
> @@ +69,3 @@
> +    }
> +
> +    public override bool is_subtype_of (DataType ancestor) {
> 
> Why did you need this new method?
> 

Hmm. It was used in previous version and erroneously it was not used (and not updated) anymore. It's needed for detection:

Set<H> s = new HashSet<G> ();

> @@ +70,3 @@
> +
> +    public override bool is_subtype_of (DataType ancestor) {
> +        if (base.is_subtype_of (ancestor)) {
> 
> Can base be null here?

Base is parent object so it's rather unlikely.

Created attachment 251148 [details] [review]
0001-Consider-the-generics-while-checking-the-validity-of.patch

(In reply to comment #19)
> (In reply to comment #18)
> > Also could you explain in detail what the "deprecated implicit conversion" mean
> 
> It is suppose to mean that a conversion between generics is made. I.e.
> 
> HashSet<HashSet<G>> hs = new HashSey<HashSet<H>>();
> 

I've forgotten to add that had forgotten to add that the warning messages need improvements and I knew it.

> > Review of attachment 251073 [details] [review] [details]:
> > 
> > Thanks. What about compatible (DataType target_type, bool strict = false,
> > SourceReference? reference = false) ?
> 
> That was my first try but the SourceReference? reference was always at the
> beginning. I don't care either way.
> 

During updating the patch I thought about it a bit more. It looks like the 'strict' might not be the best name and the parameters were to get all the changes during the compilation.

My guess the enum would be better way, especially that we want several outcomes:

 - is_subtype_of is used during cycle detection which should ignore generics. Similarly in codegen don't care as much about them
 - At least for some time the warnings are emitted instead of errors. However the caller is better equipped with emitting meaningful warnings (and errors) then calee.

> > ::: vala/valaobjecttype.vala
> > @@ +69,3 @@
> > +    }
> > +
> > +    public override bool is_subtype_of (DataType ancestor) {
> > 
> > Why did you need this new method?
> > 
> 
> Hmm. It was used in previous version and erroneously it was not used (and not
> updated) anymore. It's needed for detection:
> 
> Set<H> s = new HashSet<G> ();
>

I made an error in this comment- it was used but the generics lacked the equals function overloaded. Now it should be fixed.

Regarding why - this function compares also the generic types. So far for equality only but I hope to add upper and lower bound. So previously ArrayList<Gtk.Window> was a subtype of List<GLib.Object> or even List<string> while currently it is not (well - sort of - as it emits a warning).

Created attachment 251149 [details] [review]
0002-Remove-the-new-warnings-from-Vala-code.patch

Created attachment 251397 [details] [review]
0001-Consider-the-generics-while-checking-the-validity-of.patch

The code after moving to caller warning handling + flags.

Created attachment 251398 [details] [review]
0002-Remove-the-new-warnings-from-Vala-code.patch

*** Bug 731017 has been marked as a duplicate of this bug. ***

There's this code in DataType.compatible:

		/* temporarily ignore type parameters */
		if (target_type.type_parameter != null) {
			return true;
		}

If this is replaced by something sensible, the example in the original report doesn't compile any longer. So I wonder what those more complex patches that were added to this report do? Perhaps the code has changed since then so they're not needed any longer?

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/376.