GNOME Bugzilla – Bug 779038
Fail to discover conflicting inherited properties of interfaces and base-class
Last modified: 2018-05-22 15:46:12 UTC
Created attachment 346400 [details] [review] Fix parent interface with no setter warning If you have a parent interface with no setter in a property a warning is riced: ** (valac:9694): CRITICAL **: vala_gtype_module_cast_property_accessor_pointer: assertion 'acc != NULL' failed
Thanks for the patch. Can you include a test case in tests/objects/ The test runner exports G_DEBUG=fatal_warnings so your test should pass with this patch.
Created attachment 346473 [details] [review] Added test case This code demonstrate how warning is coming, but adding this test case an applying proposed fix fails for other reasons. After a few study of test case, I can see my implementation is wrong, because I have duplicated properties in two interfaces implemented one for parent class and required to be implemented in GText class, this should be avoided and is a programming error.
While I target to 1.2 milestone, I'll check if I can fix the problem by searching for duplicated properties and check if property has same accessors.
Created attachment 346484 [details] Set of patches to fix bug 779038 This patches add checks for implemented properties in interfaces and check parent classes accessors, rising errors if they are not compatible on readable/writable.
(In reply to Daniel Espinosa from comment #4) > Created attachment 346484 [details] This is getting better, thanks. Ideally the tests and fix would be in a single patch, no need for an archive. One of the patches, 0002-Removing-un-necesary-code-test-for-Bug-779038.patch, looks like it is a fix for the previous patch. This is where git rebase --interactive is really useful. A reviewer doesn't need to see your changes as you worked through fixing the bug. Use fixup with a rebase and it is easy to tidy that up. You can also do that to make a single patch - fix and tests in a single patch. Your test looks a little complicated and also have comments about GXml. A test case should be the simplest possible example. If the bug occurs when an interface has a parent interface then two interfaces should be enough. Some test have three, why? Invalid code tests are for the Vala compiler to report that the code is not correct, for example a syntax or other kind of error. It is for code that should not be allowed to compile. I'm not immediately seeing what is wrong with your invalid code test, apart from looking overly complex.
Example starts simple but turns little complicate when added combinations, as it provides. Code should not compile because, in the example, there are interfaces with same property name but they are read-only or write-only or read-write, then compiler should advise this situation as is an error I think. GXml have this issue, already fixed, but works, that's why may this can be a warning. I can remove some GXml related if needed.
Created attachment 346519 [details] [review] patch in one file. Fix Bug 779038
The Property class already contains checks to find these issues. Although the problem here is that two interfaces define conflicting properties and this case isn't detected. There is a FIXME marking the source of the problem. private void find_base_interface_property (Class cl) { // FIXME report error if multiple possible base properties are found ... }
*** Bug 760649 has been marked as a duplicate of this bug. ***
Could you review https://bugzilla.gnome.org/attachment.cgi?id=346519&action=diff It provides error thrown when detecting conflictive properties setters and getters. If necessary please tell me how to improve it.
Review of attachment 346519 [details] [review]: https://bugzilla.gnome.org/show_bug.cgi?id=779038#c8
Created attachment 347647 [details] [review] class: Perform more thorough compatibility check of inherited properties
I have taken a look to reuse the existing Property.compatible() method.
Review of attachment 347647 [details] [review]: May is a good idea to add more test cases with getter/setter combinations, in order to check this patch works properly.
Yeah, I will add 2 more invalid-code checks to cover data-type and set-accessors problems. Bad news is that libgee and shotwell are failing in a bad way: http://paldo.org:8010/builders/vala-staging/builds/232
Created attachment 347659 [details] [review] class: Perform more thorough compatibility check of inherited properties
Review of attachment 347659 [details] [review]: Is Ok for me.
Attachment 347659 [details] pushed as e874bb7 - class: Perform more thorough compatibility check of inherited properties
This broke Builder's builds.
(In reply to Christian Hergert from comment #19) > This broke Builder's builds. Is gnome-builder itself building fine? It is here: http://paldo.org:8010/builders/vala-staging/builds/233/steps/gnome-builder Are you getting a runtime critical or segfault when the user selects Build? Please give details of what is broken.
http://sdkbuilder1.gnome.org//logs/build-2017-03-10-150708/build-gnome-apps-nightly-master-org.gnome.Builder-i386.txt Ide.Object has more relaxed guarantees than the interface (but should be fine). If I instead go to not using our helper base class and instead implement the interface properties in Vala, I get multiple definitions of the same function foo_real_set_bar. 8< ./ide-vala-completion-provider.vala:25.2-27.60: error: Type and/or accessors of inherited properties `Ide.CompletionProvider.context' and `Ide.Object.context' do not match: incompatible get accessor. ./ide-vala-service.vala:25.2-25.50: error: Type and/or accessors of inherited properties `Ide.Service.context' and `Ide.Object.context' do not match: incompatible get accessor. public class ValaService: Ide.Object, Ide.Service ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ./ide-vala-diagnostic-provider.vala:25.2-25.72: error: Type and/or accessors of inherited properties `Ide.DiagnosticProvider.context' and `Ide.Object.context' do not match: incompatible get accessor. public class ValaDiagnosticProvider: Ide.Object, Ide.DiagnosticProvider ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ./ide-vala-indenter.vala:24.2-24.52: error: Type and/or accessors of inherited properties `Ide.Indenter.context' and `Ide.Object.context' do not match: incompatible get accessor. public class ValaIndenter: Ide.Object, Ide.Indenter ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ./ide-vala-pipeline-addin.vala:26.2-26.67: error: Type and/or accessors of inherited properties `Ide.BuildPipelineAddin.context' and `Ide.Object.context' do not match: incompatible get accessor type. public class ValaPipelineAddin: Ide.Object, Ide.BuildPipelineAddin ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ./ide-vala-symbol-resolver.vala:25.2-25.64: error: Type and/or accessors of inherited properties `Ide.SymbolResolver.context' and `Ide.Object.context' do not match: incompatible get accessor. public class ValaSymbolResolver: Ide.Object, Ide.SymbolResolver ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >8
(In reply to Al Thomas from comment #20) > Is gnome-builder itself building fine? It is here: > http://paldo.org:8010/builders/vala-staging/builds/233/steps/gnome-builder It looks like that auto-builder is doing incremental builds. Which means it's not regenerating .[ch] files with valac unless they've changed. The autobuilder should be updated to either: 1) Make clean that directory (which we have set to remove .[ch] files) 2) Clean and do full rebuilds each time
(In reply to Christian Hergert from comment #21) > Ide.Object has more relaxed guarantees than the interface (but should be > fine). Looking in more detail at this one: > ./ide-vala-completion-provider.vala:25.2-27.60: error: Type and/or accessors > of inherited properties `Ide.CompletionProvider.context' and > `Ide.Object.context' do not match: incompatible get accessor. Looks like the Vala code implements both Ide.Object and Ide.Service: https://git.gnome.org/browse/gnome-builder/tree/plugins/vala-pack/ide-vala-service.vala Ide.Object and Ide.Service are implemented in C. The Vala code is 'using Ide;'. Looks like a VAPI built from a GIR. As a side note there is the Builder GIR here: https://github.com/nemequ/vala-girs/blob/master/gir-1.0/Builder-1.0.gir but it doesn't seem to be used by valadoc.org for documentation. The Ide.Object.context GParamSpec has G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS: https://git.gnome.org/browse/gnome-builder/tree/libide/ide-object.c#n214 The Ide.Service.context GParamSpec has G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS: https://git.gnome.org/browse/gnome-builder/tree/libide/ide-service.c#n78 So I assume that is why there is an incompatible get accessor, because Ide.Service.context is write only?
(In reply to Al Thomas from comment #23) > (In reply to Christian Hergert from comment #21) > > Ide.Object has more relaxed guarantees than the interface (but should be > > fine). > > Looking in more detail at this one: > > ./ide-vala-completion-provider.vala:25.2-27.60: error: Type and/or accessors > > of inherited properties `Ide.CompletionProvider.context' and > > `Ide.Object.context' do not match: incompatible get accessor. > > Looks like the Vala code implements both Ide.Object and Ide.Service: > https://git.gnome.org/browse/gnome-builder/tree/plugins/vala-pack/ide-vala- > service.vala > > Ide.Object and Ide.Service are implemented in C. The Vala code is 'using > Ide;'. Looks like a VAPI built from a GIR. As a side note there is the > Builder GIR here: > https://github.com/nemequ/vala-girs/blob/master/gir-1.0/Builder-1.0.gir > but it doesn't seem to be used by valadoc.org for documentation. > > The Ide.Object.context GParamSpec has G_PARAM_READWRITE | > G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS: > https://git.gnome.org/browse/gnome-builder/tree/libide/ide-object.c#n214 > The Ide.Service.context GParamSpec has G_PARAM_WRITABLE | > G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS: > https://git.gnome.org/browse/gnome-builder/tree/libide/ide-service.c#n78 > > So I assume that is why there is an incompatible get accessor, because > Ide.Service.context is write only? This is exactly the situation to be detected by this patch. Then It should be checkout by Builder in order to provide compatible accessors. Now the question could be: Is this a real error or should be downgraded to warning?
(In reply to Christian Hergert from comment #22) > (In reply to Al Thomas from comment #20) > > Is gnome-builder itself building fine? It is here: > > http://paldo.org:8010/builders/vala-staging/builds/233/steps/gnome-builder > > It looks like that auto-builder is doing incremental builds. Which means > it's not regenerating .[ch] files with valac unless they've changed. > > The autobuilder should be updated to either: > > 1) Make clean that directory (which we have set to remove .[ch] files) > 2) Clean and do full rebuilds each time I don't think this is necessary if you use vala sources as dependencies of .[ch], like in GXml (very ugly code): vala-stamp: $(sources) @rm -f vala-temp @touch vala-temp $(VALAC) $(AM_VALAFLAGS) $^ @mv -f vala-temp $@ $(sources:.vala=.c): vala-stamp ## Recover from the removal of $@ @if test -f $@; then :; else \ trap ’rm -rf vala-lock vala-stamp’ 1 2 13 15; \ if mkdir vala-lock 2>/dev/null; then \ ## This code is being executed by the first process. rm -f vala-stamp; \ $(MAKE) $(AM_MAKEFLAGS) vala-stamp; \ rmdir vala-lock; \ else \ ## This code is being executed by the follower processes. ## Wait until the first process is done. while test -d vala-lock; do sleep 1; done; \ ## Succeed if and only if the first process succeeded. test -f vala-stamp; exit $$?; \ fi; \ fi Some of this stuff are encapsulated in .mk macros, with an skeleton like at: https://github.com/esodan/automakevala/blob/master/vala.mk
(In reply to Al Thomas from comment #23) > So I assume that is why there is an incompatible get accessor, because > Ide.Service.context is write only? G_PARAM_WRITABLE is not "write-only" per-se. It is just that it's writable. So if you have an interface that requires something is at least writable, it is totally reasonable for that implementation to also implement readable at it's own discretion.
I think this should be reverted. Vala should: A) implement just only compatible properties B) rise error on unimplemented properties C) per class, generate one method per implemented abstract method, per abstract class and GInterface, so no warning should be rise by C compiler. Each one calling the *real* method. A) and B) are already in place before this change. C) needs works. So if Foo is a property of A implementing abstract class B's' readonly property FooB and read-write Interface C's FooC property, then we ended with: 1) getter and setter public methods using A as parameter for self 2) one getter private implementor method using B as parameter for self 3) getter and setter private implementor methods using C as parameter for self 4) real getter and setter pumrivate methods using A as parameter for self, called by above methods.
Doing this additional check is correct since this just enforces what vala expects in all other situations. This is about whether to relax it to the way gobject allows it. See https://bugzilla.gnome.org/show_bug.cgi?id=686542
-- 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/579.