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 779038 - Fail to discover conflicting inherited properties of interfaces and base-class
Fail to discover conflicting inherited properties of interfaces and base-class
Status: RESOLVED OBSOLETE
Product: vala
Classification: Core
Component: Code Generator: GType
unspecified
Other All
: Normal enhancement
: 1.2
Assigned To: Vala maintainers
Vala maintainers
: 760649 (view as bug list)
Depends on:
Blocks: 686542
 
 
Reported: 2017-02-21 20:53 UTC by Daniel Espinosa
Modified: 2018-05-22 15:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix parent interface with no setter warning (1.79 KB, patch)
2017-02-21 20:53 UTC, Daniel Espinosa
none Details | Review
Added test case (1.46 KB, patch)
2017-02-22 18:30 UTC, Daniel Espinosa
none Details | Review
Set of patches to fix bug 779038 (1.68 KB, application/x-xz)
2017-02-22 19:33 UTC, Daniel Espinosa
  Details
patch in one file. Fix Bug 779038 (6.26 KB, patch)
2017-02-22 23:28 UTC, Daniel Espinosa
needs-work Details | Review
class: Perform more thorough compatibility check of inherited properties (2.12 KB, patch)
2017-03-10 15:00 UTC, Rico Tzschichholz
none Details | Review
class: Perform more thorough compatibility check of inherited properties (3.48 KB, patch)
2017-03-10 17:35 UTC, Rico Tzschichholz
committed Details | Review

Description Daniel Espinosa 2017-02-21 20:53:03 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
Comment 1 Al Thomas 2017-02-21 21:05:54 UTC
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.
Comment 2 Daniel Espinosa 2017-02-22 18:30:30 UTC
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.
Comment 3 Daniel Espinosa 2017-02-22 18:32:37 UTC
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.
Comment 4 Daniel Espinosa 2017-02-22 19:33:55 UTC
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.
Comment 5 Al Thomas 2017-02-22 20:21:43 UTC
(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.
Comment 6 Daniel Espinosa 2017-02-22 21:47:54 UTC
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.
Comment 7 Daniel Espinosa 2017-02-22 23:28:32 UTC
Created attachment 346519 [details] [review]
patch in one file. Fix Bug 779038
Comment 8 Rico Tzschichholz 2017-03-07 22:46:18 UTC
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
...
}
Comment 9 Rico Tzschichholz 2017-03-09 21:04:42 UTC
*** Bug 760649 has been marked as a duplicate of this bug. ***
Comment 10 Daniel Espinosa 2017-03-09 21:34:41 UTC
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.
Comment 12 Rico Tzschichholz 2017-03-10 15:00:58 UTC
Created attachment 347647 [details] [review]
class: Perform more thorough compatibility check of inherited properties
Comment 13 Rico Tzschichholz 2017-03-10 15:02:22 UTC
I have taken a look to reuse the existing Property.compatible() method.
Comment 14 Daniel Espinosa 2017-03-10 15:11:46 UTC
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.
Comment 15 Rico Tzschichholz 2017-03-10 15:27:52 UTC
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
Comment 16 Rico Tzschichholz 2017-03-10 17:35:41 UTC
Created attachment 347659 [details] [review]
class: Perform more thorough compatibility check of inherited properties
Comment 17 Daniel Espinosa 2017-03-10 17:44:57 UTC
Review of attachment 347659 [details] [review]:

Is Ok for me.
Comment 18 Rico Tzschichholz 2017-03-10 18:28:25 UTC
Attachment 347659 [details] pushed as e874bb7 - class: Perform more thorough compatibility check of inherited properties
Comment 19 Christian Hergert 2017-03-10 21:26:46 UTC
This broke Builder's builds.
Comment 20 Al Thomas 2017-03-10 21:40:11 UTC
(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.
Comment 21 Christian Hergert 2017-03-10 22:02:56 UTC
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
Comment 22 Christian Hergert 2017-03-10 22:08:24 UTC
(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
Comment 23 Al Thomas 2017-03-10 22:48:11 UTC
(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?
Comment 24 Daniel Espinosa 2017-03-10 22:54:56 UTC
(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?
Comment 25 Daniel Espinosa 2017-03-10 22:55:35 UTC
(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
Comment 26 Christian Hergert 2017-03-10 23:03:36 UTC
(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.
Comment 27 Daniel Espinosa 2017-03-14 15:46:31 UTC
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.
Comment 28 Rico Tzschichholz 2017-03-14 16:27:24 UTC
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
Comment 29 GNOME Infrastructure Team 2018-05-22 15:46:12 UTC
-- 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.