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

Created attachment 171039 [details] [review]
Enhance checking of generic type arguments.

Fixes bug 615830.

I've looked at the path and I believe it isn't entirely correct. Two generic types are compatible only if the type arguments are exact match. For example, you can't pass List<Widget> as List<Object>, because that would mean you could add some non-widget to a list of widgets.
Requiring exact match is inflexible, but Vala is missing syntax that could solve this problem.

Created attachment 171103 [details] [review]
Enhance checking of generic type arguments.

Fixes bug 615830.

You're totally right, I've missed that. New patch checks for equality instead of compatibility.

It should be noted that this patch will probably break some code. There are many valid usages that won't pass this check, because as I said, the syntax to allow more flexible typing is missing in Vala.

Created attachment 175405 [details] [review]
Enhance checking of generic type arguments

This will check for nested generics as well.

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

Created attachment 194562 [details] [review]
Improve compatibility check for generic type arguments

Check for generic type arguments compatibility when the target type
is a subtype of this. Also enforce ownership of generic type arguments
to match target generic type arguments to avoid memory management issues.

Partially fixes 615830.

Created attachment 332950 [details] [review]
fixes this bug

Hey guys,

I noticed this bug independently, fixed it and then found this bug report. I'm attaching a patch, it would be great if somebody could take a look…

Cheers,
Matthias

Review of attachment 332950 [details] [review]:

Causes an error when compiling SQLHeavy (https://github.com/nemequ/sqlheavy):

+ Trace 236515

Hey Evan,

thanks, that's very helpful. Could you give me some instructions on how to reproduce the issue? Specifically, is there a way to tell sqlheavy's build system which valac to use?

Ah, never mind, make VALAC=$HOME/vala/compiler/valac VALAFLAGS="--vapidir $HOME/vala/vapi" did the trick :-)

OK, the problem seems to be caused by this line:

    private extern unowned GLib.HashTable g_hash_table_ref (GLib.HashTable ht);

The problem here is that the type parameters are simply not specified for the HashTable so the type parameters are just not given. IMO the compiler shouldn't accept code like that at all.

My suggestion would be to deprecate this syntax since adding the machinery to support this (probably some form of existential types) is IMO not worth the effort. The above declaration could easily be replaced by something like

    private extern unowned <K, V> GLib.HashTable<K, V> g_hash_table_ref (GLib.HashTable<K, V> ht);


In the meantime, I'll add some workaround for the crash.

Created attachment 333079 [details] [review]
bugfix + testcase

sqlheavy should now compile. I've added a test for the behaviour that sqlheavy relies on (that one doesn't have to specify generic type parameters) since that currently seems to be the only way to bind functions like g_hash_table_ref correctly. It's super ugly but for the time being I don't see any alternative.

Created attachment 333109 [details] [review]
bugfix + testcase

Simplified the code a bit

Review of attachment 333109 [details] [review]:

Patch applies and works for me. Rebuilt vala with the patch applied and no problems. All tests passed.

Review of attachment 333109 [details] [review]:

::: vala/valadatatype.vala
@@ +305,3 @@
 
+		if (data_type == null && target_type.data_type == null)
+			return true;

Do you know of a case where this is required or did you simply want to preserve the behavior when the two data_type members are both null?

I would rather drop this.

Hey Jürg,

Thanks for the review.

I don't know if such a case, so feel free to drop it.

*of* such a case, that is 

Created attachment 335401 [details] [review]
Updated patch

The patch breaks build of libgee and shotwell.

libgee/treemap.vala:1975.5-1975.20: error: Assignment: Cannot convert from `Gee.TreeMap.SubEntryIterator<K,V>' to `Gee.Iterator<Gee.TreeMap.Entry<K,V>>'

shotwell/src/core/ViewCollection.vala:649.30-649.37: error: Argument 1: Cannot convert from `Gee.Set<DataObject>' to `Gee.Collection<DataView>'
shotwell/src/MediaMonitor.vala:352.50-352.58: error: Argument 1: Cannot convert from `Gee.ArrayList<MediaSource>?' to `Gee.Collection<Monitorable>?'

These may actually be bugs in libgee and shotwell, respectively, however, I can't push this patch to vala master at this point with these pending issues.

It also breaks folks:

anti-linkable.vala:135.37-135.50: error: Argument 1: Cannot convert from `Folks.SmallSet<G>' to `Gee.Set<string>'

Those breakages might be unfortunate and actual coding-errors even, we can't ignore them since e.g. libgee is a core library and fixing this problem likely leads to an ABI break.

libgee is a core library, but aiui shotwell and folks are not. So does that imply that when libgee is fixed, this fix can be go to master if libgee is fixed?

Besides, this change will not result in an ABI break. It doesn't touch code generation at all and type errors can simply be resolved with a cast which aiui also doesn't affect code generation.

I would also like to propose that we actually _ask_ users what they would rather have: a compiler that tells them about the silly mistakes that they make _now and in the future_, or one that silently ignores all such errors in order to save them from correcting one that they made in the past. I don't think it's wise to simply decide that for them based on assumptions that nobody bothered to check.

I am refering to a possible ABI breaks of e.g. libgee while fixingsurfaced issues.

I don't understand. The bugs in libgee can be fixed by adding casts. How is this supposed to affect ABI compatibility? Surely casting some reference to another type is not going to affect the value of the underlying pointer?!

> These may actually be bugs in libgee and shotwell, respectively, however, I
> can't push this patch to vala master at this point with these pending issues.

I'll have a look at shotwell

 (valac:29658): GLib-GObject-WARNING **: invalid cast from 'ValaStruct' to 
'ValaInterface'   

I'm getting that warning with the patch and the code at wip/615830 (shotwell repository)

(In reply to Matthias Berndt from comment #23)
> I would also like to propose that we actually _ask_ users what they would
> rather have: a compiler that tells them about the silly mistakes that they
> make _now and in the future_, or one that silently ignores all such errors
> in order to save them from correcting one that they made in the past. I
> don't think it's wise to simply decide that for them based on assumptions
> that nobody bothered to check.

The missing check should definitely be merged. What I meant was that it would be very bad to merge it a few hours before hard code freeze, knowingly breaking the build of multiple projects. Also, we should verify that these are easily fixable bugs in application/library code and not issues in the compiler - at least for libgee and projects with a maintainer willing to help, such as shotwell.

We should merge this after 0.34.0 as soon as the above is fixed.

Ok, glad to hear this!

(In reply to Jens Georg from comment #27)
>  (valac:29658): GLib-GObject-WARNING **: invalid cast from 'ValaStruct' to 
> 'ValaInterface'   
> 
> I'm getting that warning with the patch and the code at wip/615830 (shotwell
> repository)
That's interesting. Can you provide more details? What revision have you compiled, what flags did you pass to configure, autogen.sh and make and which file caused that message to be printed?

commit de38563e742fcf4041779ea19ec9458f57fa3879 + the patch from this bug

which configure/autogen.sh do you mean? The problem with shotwell is that everything is compiled in one go so I can't tell you the file :(

Well, shotwell's autogen and configure of course. 

Anyhow, I think I managed to reproduce it in the mean time. I had to: 
- rip out everything to do with APPDATA_XML from the build system (apparently appdata-tools is dead and not shipped with Fedora any longer)
- export VALAC=$HOME/vala/compiler/valac
- add --vapidir $HOME/vala/vapi to COMMON_VALAFLAGS in common.am – apparently shotwell's build system doesn't believe in the VALAFLAGS env variable, oh well

Now debugging this might be a bit tricky because I have no idea what file the line number in the error message refers to…

I would suggest to run valac with G_DEBUG=fatal-warnings (or set a breakpoint), check the backtrace, and poke relevant variables with gdb, e.g., print vala_source_reference_get_file(...).

(In reply to Matthias Berndt from comment #32)
> Well, shotwell's autogen and configure of course. 
> 
> Anyhow, I think I managed to reproduce it in the mean time. I had to: 
> - rip out everything to do with APPDATA_XML from the build system
> (apparently appdata-tools is dead and not shipped with Fedora any longer)

That's called appstream-util now.

> - add --vapidir $HOME/vala/vapi to COMMON_VALAFLAGS in common.am –
> apparently shotwell's build system doesn't believe in the VALAFLAGS env
> variable, oh well

I would consider this a bug. I have to look into that.

Thanks Jürg! It seems to produce a core dump as I get the typical "Speicherabzug geschrieben" message, but I don't see a core file, do you have an idea where it might be? Getting a stack trace would be helpful indeed… I already set ulimit -c unlimited.

Sorry, I do Scala in my day job, so my skills with the typical C infrastructure are a bit rusty ;-)

Ah, nevermind, apparently systemd (coredumpctl) is in charge of core dumps these days…

If it's any help gdb can be run interactively for a backtrace:

gdb program
> run
> bt

For debug info on Fedora someone has put some details on the Vala install pages:
https://wiki.gnome.org/Projects/Vala/ValaOnLinux

Also the --debug switch in valac will annotate a C code line with the relevant Vala code line that produced it.

Hey Al,

thanks for replying. I've managed to obtain a somewhat meaningful stack trace from the core dump. The crash seems to be in SemanticAnalyzer.get_instance_base_type, specifically this bit of code

if (base_type.data_type is Class) {
  instance_base_type = new ObjectType ((Class) base_type.data_type);
} else {
  instance_base_type = new ObjectType ((Interface) base_type.data_type);
}

Now I don't know what led the original author to assume that base_type.data_type is always either a Class or an Interface, but apparently that doesn't hold any longer…

For anyone who's interested, a fairly minimal example that triggers the bug seems to be this:

struct Foo {
  public int x;
}
struct Bar : Foo {}
void main() {
  Foo f = Bar();
}

Now, that's enough for today, have a nice evening everybody :-)

So it is simply triggered by the usage of SemanticAnalyzer.get_instance_base_type_for_member() in the patch, and this method is not made for SimpleTypes/Structs.

So, I took a look at that code. Unfortunately it's a bit hard to tellif there are any types other than ObjectType and StructValueType that might be passed to get_instance_base_type_for_member. I guess we'll have to try and see. For now I've modified the code to handle Structs, I hope that'll be enough. If somebody would take a look that would be great.

I also noticed something else: There seems to be an annotation called simple_generics. Unfortunately it's not documented in the manual, but I've tried it and it seems that when calling a generic function or method annotated with it, no GType, GBoxedCopyFunc etc. are passed to it in the generated C code. Apparently the purpose is to bind functions like g_hash_table_ref, mentioned in Comment 13 and 14. That's great because then we might actually disallow code like in SQLheavy where GHashTable is used without parameters.

Alas, while it works at the function *call* site, it also generates a function *prototype* that still includes those parameters, which is unfortunate and makes me wonder if it's intended like that…

Created attachment 335952 [details] [review]
Updated patch

Created attachment 335954 [details] [review]
Updated patch

Added another test for the struct case

Created attachment 335955 [details] [review]
Updated patch

Added another test for the struct case

If it's any help the use of the simple_generics CCode attribute detail is covered here: 
https://wiki.gnome.org/Projects/Vala/LegacyBindings#Parameters_of_Variable_Type_.28Generics.29

Personally, I'm slightly uneasy that a function called get_instance_base_type_for_member is being used for structs, etc. To my mind keywords 'instance' and 'member' relate to OOP and object instantiation and the instance a function acts upon. This would explain why the original code does not cater for structs.

In general there need to be a set of utility functions in the semantic analyser for these types of query. You may have to create a new one or look for an alternative. Maybe https://bugzilla.gnome.org/show_bug.cgi?id=769501 might provide some inspiration. Thinking about this, it may even be need to be a utility function within the AST component. Other tools, such as an IDE in the future, may be using such utility functions so design now may be important later.

Hey Al,

thanks for the link. I'll look into it, but I think it's a separate issue for now.

Regarding your unease about the name, I don't see the problem to be honest. Classes and Structs in Vala have more in common than separates them. They have methods, constructors, member variables and type parameters and they form inheritance hierarchies. IOW, they behave for the most part like classes except for the limitations imposed by the runtime representation, i. e. no new fields in derived structs, no reference equality etc.. So I think it makes sense to deal with them in the same function, they're not fundamentally different from classes.

Regarding the needs of an IDE, I would very much prefer not to speculate about that. I don't see an obvious use case for such a function and would therefore default to YAGNI. It also seems to me that the primary requirement for an IDE would be a parser capable of dealing with broken input in a sensible way, and it seems pointless to worry about features beyond that for now. Let's focus on fixing what we have before taking on new use cases; there are, after all, a lot of bugs left to fix in valac.

Oh, I actually think that get_instance_base_type_for_member not working with structs is really just an oversight rather than any sort of design issue. It was possible to cause it to be called with a struct type before my patch, e. g.

struct Foo<T> {T t;}
struct Bar<T> : Foo<T> {}
void main() {
  int f = Bar<int>().t;
}

That crashes valac 0.32.1. It's now just called in cases it wasn't called before.

Could you check also more complicated use-cases (skimming through initial patches it looked like it might not be handled). Like:

class Foo<K, V> {
    public abstract V foo(K k);
}

class Bar<V, K> : Foo<V, K> {
    override K foo(V v) {
        return null;
    }
}

class FooBar<K> : Bar<K, Foo<K, K>> {
    public K bar(K k) {
        return foo(k).foo(k);
    }
}

(In reply to Matthias Berndt from comment #46)
> Regarding the needs of an IDE...I don't see an obvious use case for such a
> function...It also seems to me that the primary requirement
> for an IDE would be a parser capable of dealing with broken input in a
> sensible way, and it seems pointless to worry about features beyond that for
> now.

The feature is autocompletion and function/method parameter information. So when using an object the IDE can find the inherited methods and show them for autocompletion and also show the arguments needed. Whether this function helps with that I don't know.

> Let's focus on fixing what we have before taking on new use cases;
> there are, after all, a lot of bugs left to fix in valac.

valac is a front end to libvala. A lot of tools, such as valadoc, anjuta, GNOME Builder, etc. use libvala for extracting information from Vala source files. So that use case already exists. e.g. https://bugzilla.gnome.org/show_bug.cgi?id=768817 At the moment I'm thinking another front end should exist, a libvala-ide-tools shared object, that provides such services in a stable API/ABI. I think this is something to consider when looking deeply at the libvala source code.

Hey Al,

you're right, the function may in fact be useful for that sort of purpose. However I frankly don't see how fixing it to work with structs makes it any less useful for those purposes than it already is. There may be ways to make it even more useful, e. g. by sticking it into a separate library, but I believe that's out of scope for this bug.

Hey Maciej,

could you go into a bit more detail on your example? Afaics it doesn't contain any type errors, how is it related to this bug?

The only thing that's interesting about this is that you flip the names of the type arguments, which might confuse the compiler. I actually fixed one source of compiler confusion with my patch to #768823 and there is a test case for that, maybe that is enough?

(In reply to Matthias Berndt from comment #50)
> you're right, the function may in fact be useful for that sort of purpose.
> However I frankly don't see how fixing it to work with structs makes it any
> less useful for those purposes than it already is.

Functionally there is no problem of course. It is naming things so code is self-documenting as much as possible. I'm probably getting confused between objects and basic types, but, yes, basic types in Vala are structs and have members. I don't think enums can inherit, but they also have members. If that needs testing as well, but enums are ints and so probably structs as well. I'm not sure about delegates.

> There may be ways to make it even more useful, e. g. by sticking it into
> a separate library, but I believe that's out of scope for this bug.

Sure, it is out of scope. Also I wasn't proposing to move the function out of libvala. The idea of libvala-ide-tools would be to use the existing libvala functions, but provide API/ABI stability, a friendlier high level API and probably some kind of session.

(In reply to Al Thomas from comment #52)
> Functionally there is no problem of course. It is naming things so code is
> self-documenting as much as possible.
So since your only gripe seems to be the name, could you suggest a better one?

(In reply to Matthias Berndt from comment #53)
> So since your only gripe seems to be the name
?? I was pointing out some strategic direction for future development, but those are long term goals. As far as the name goes, instead of changing the function from private to public, I suggest changing it to internal. Then when https://bugzilla.gnome.org/show_bug.cgi?id=771920 goes in it won't appear in libvala's dynamic symbol table and potentially get used by IDE writers.

I've looked through the Vala code base and wondered why the patch adds the check to DataType.compatible () and not Method.compatible () ,but no-one else has questioned that so I will look further another time.

As far as your patch goes: 

 - Why is instance_base_type = new ObjectType ((Interface) base_type.data_type); not necessary at all?
 - instance_base_type = null; is not needed if you are then going to assert or is that just to get past a compiler warning?
 - should the assert be assert_not_reached (); but really that is debugging code because as you said earlier "Unfortunately it's a bit hard to tellif there are any types other than ObjectType and StructValueType that might be passed to get_instance_base_type_for_member. I guess we'll have to try and see."
 - there should be a space between assert and the parentheses - https://wiki.gnome.org/Projects/Vala/Hacking#Coding_Style

BTW, your patch is in staging:

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

That includes output from the failed builds if it is any help

Hey Al,

thanks, I didn't know about staging. That is helpful indeed! I couldn't get libgee's build system to work on my system (it complains about not finding geeutils, whatever that is), so it's useful to see the actual error messages. After checking the source code I believe they're legitimate.

Regarding your questions:
- there was no reason to treat Interfaces and Classes separately in the first place since either way an ObjectType is created (and not a ClassType or InterfaceType, which also exist but don't seem to be used here). So I could simplify that code by simply testing for and casting to ObjectType rather than treat Interfaces and Classes separately. Structs on the other hand are treated differently.
- right, I put that there to make it compile. With assert_not_reached that is not needed any longer :-). I'll upload a new version with that and the whitespace fix.

Created attachment 336215 [details] [review]
patch

Anyone got any thoughts on get_instance_base_type_for_member being 'internal' instead of 'public'?

Matthias, thanks for the quick response. If you do another iteration then g_assert_not_reached (); doesn't take an argument.

Created attachment 336226 [details] [review]
patch

made get_instance_base_type_for_member internal

Created attachment 336227 [details] [review]
patch

I'm a bit surprised because I thought I had compiled that code, yet it still had the argument to assert_not_reached… anyway, that's fixed now.

Hey Al,

I have also looked at method.compatible. It is broken indeed and needs to be fixed, but I wonder if it needs to be done in the context of this bug? Let's fix one problem at a time, I would really like to keep the scope small and fix one problem at a time.

We can discuss Method.compatible in bug 641418.

libgee master now builds with this patch.

Jens, can you please look into these issues in shotwell with vala master and the latest patch (currently in staging)?
http://paldo.org:8010/builders/vala-staging/builds/100/steps/shotwell/logs/stdio

Can you check the wip/615830 branch of shotwell? I didn't push it to master because of the warnings in comment 27

Otherwise I try to do it later but can't promise

Works on master now

Comment on attachment 336227 [details] [review]
patch

Pushed to master.

This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.