Created attachment 163612 [details] [review]
Support scriptable=no attribute

I don't like the name "scriptable". The typelib is used by non-"scripting" languages too.

Yes, "introspectable=no" sucks, and no I don't have a better suggestion

Review of attachment 163612 [details] [review]:

Didn't review most of girparser.c which I think should be moved to a separate patch.

::: giscanner/annotationparser.py
@@ +869,3 @@
     def _parse_skip(self, node, block):
         if block is not None:
             if OPT_SKIP in block.options:

Should we update the annotation option as well?

::: giscanner/girwriter.py
@@ +134,3 @@
             self.write_tag('attribute', [('name', key), ('value', value)])
 
+    def _append_node_generic(self, node, attrs):

Just _append_node() ?

@@ +136,3 @@
+    def _append_node_generic(self, node, attrs):
+        if not node.scriptable:
+            attrs.append(('scriptable', 'no'))

We use 1/0 pretty consistently here.

::: giscanner/glibtransformer.py
@@ +188,3 @@
             self._resolve_types(nodes)
 
+        self._validate(nodes)

Needs to be disabled by default until warnings/error reporting system is in place.

@@ +848,3 @@
+    # Node walking
+
+    def _walk(self, node, callback, chain):

Getting pretty tired of seeing all these node walking routines. We need to clean them up so we don't traverse the tree 5-6 times or so like we do today.

@@ +1113,3 @@
+            for vfunc in node.virtual_methods:
+                if not vfunc.invoker:
+                    print ("warning: Interface %r virtual function %r " + \

No need for \

@@ +1142,3 @@
     def _validate(self, nodes):
         for (name, node) in nodes:
+            self._walk(node, self._validate_node, [])

What about an api like this;
self._walk(node, callback, [list_of_types_that_matches]), eg:

def walk_type_container(node, stack):
   if type container
     do _scriptable_analysis
     do _scriptable_pass2
   if glib interface
     do _interface_vfunc_check

self._walk(node, walk_type_container, [TypeContainer, GLibInterface])

Which avoids traversing the tree twice.

I agree with Dan that 'scriptable' is not a very good name. Couple of naming suggestions;
  * exported
  * public
  * published
  * supported
  * visible

Possibly with with a "api" prefix/suffix.

(In reply to comment #4)
> I agree with Dan that 'scriptable' is not a very good name. Couple of naming
> suggestions;
>   * exported
>   * public
>   * published
>   * supported
>   * visible

The problem is that for e.g. functions which take varargs, unannotated GList * returns, they're *all* of those things for people using from C directly.

I'll go with "introspectable" I guess if you guys feel strongly.  It's less obvious though than "scriptable" - which is by far the most important use of introspection data, even though I agree it's not the only one.

(In reply to comment #2)
> I don't like the name "scriptable". The typelib is used by non-"scripting"
> languages too.

If this is the precise objection - while I know the Java bindings I was prototyping worked that way, it makes more sense if one is statically generating code to work from the .gir, I think.

Well, when I saw the bug summary, my first thought was that "scriptable=no" meant either "only for interactive/command-line/REPL use" or "not to be used when implementing dbus actions for other apps to invoke a la AppleScript". Which are both nonsensical things to annotate, but I couldn't think of anything else "scriptable" could refer to. I eventually figured it out after reading the description of the behavior, but if I had been first exposed to it by coming across the annotation in some source code, I would never have correctly guessed what it did.

(In reply to comment #7)
> Well, when I saw the bug summary, my first thought was that "scriptable=no"
> meant either "only for interactive/command-line/REPL use" or "not to be used
> when implementing dbus actions for other apps to invoke a la AppleScript".
> Which are both nonsensical things to annotate, but I couldn't think of anything
> else "scriptable" could refer to. I eventually figured it out after reading the
> description of the behavior, but if I had been first exposed to it by coming
> across the annotation in some source code, I would never have correctly guessed
> what it did.

Ah, well the source code annotation is actually (skip).  We could use that too in the GIR I guess.

Created attachment 163703 [details] [review]
Add some more Gio annotations

Created attachment 163704 [details] [review]
[giscanner] Apply annotations from invoker to vfunc

We typically expect people to annotate e.g. GList for virtuals
on the invoker, not on the virtual slot, since the invoker feels
like the public API.

Created attachment 163705 [details] [review]
Support introspectable=no attribute, add warnings framework

This work allows us to move closer to replacing gtk-doc, among other
things.  We add a generic attribute "introspectable", and inside the
typelib compiler if we see "introspectable=no", we don't put it in the
typelib.  This replaces the hackish pre-filter for varargs with a much
more generic mechanism.

The varargs is now handled in the scanner, and we emit
introspectable=no for them.

Add generic metadata to Node with references to file/line/column,
which currently comes from symbols.

Add scanner options --Wintrospectable and --Wfatal.

(In reply to comment #11)
> Created an attachment (id=163705) [details] [review]
> Support introspectable=no attribute, add warnings framework

This is now squashed with my warnings patch, because while they are sort of conceptually independent in practice they touch too many related areas.

The warnings patch is fairly small right now - but we need to add many more warnings, such as missing (transfer) on return values.

You can see examples of the warnings when scanning Gio.

Review of attachment 163703 [details] [review]:

Yes

Review of attachment 163704 [details] [review]:

Looks good.

Review of attachment 163705 [details] [review]:

::: giscanner/glibtransformer.py
@@ +161,3 @@
                 self._resolve_node(node)
             except KeyError, e:
+                print "WARNING: DELETING node %s: %s" % (node.name, e)

Remove this, or make it into warning

::: giscanner/scannermain.py
@@ +88,3 @@
                       action="append", dest="packages_export", default=[],
                       help="Associated pkg-config packages for this library")
+    parser.add_option('', "--Wintrospection",

Do something like this instead, do be able to get -W and multiple options;

parser.add_option('-W', '', action="append", dest="warnings", default=[],
                  "pass a warning")

If you pass in -Wall -Wsliff-sloff options.warnings will then contain ['all', 'sliff-sloff'].

Everything in that list should be verified, and a warning should be printed if there's an unknown name instead of being fatal, eg: Warning: Unknown option -Wfoobar.

Remember that all warnings needs to be documented and added to the manual page.

::: giscanner/transformer.py
@@ +125,3 @@
     # Private
 
+    def _log_node_warning(self, node, string, context=None):

This should be moved out of transformer.py and be a public function

giscanner/log:warn(node, string, context) or so.

Can probably be more flexible, to allow the callsites of this function to do most of the string formatting.

(In reply to comment #15)
> Review of attachment 163705 [details] [review]:
> 
> ::: giscanner/glibtransformer.py
> @@ +161,3 @@
>                  self._resolve_node(node)
>              except KeyError, e:
> +                print "WARNING: DELETING node %s: %s" % (node.name, e)
> 
> Remove this, or make it into warning

Done.

> 
> ::: giscanner/scannermain.py
> @@ +88,3 @@
>                        action="append", dest="packages_export", default=[],
>                        help="Associated pkg-config packages for this library")
> +    parser.add_option('', "--Wintrospection",
> 
> Do something like this instead, do be able to get -W and multiple options;
> 
> parser.add_option('-W', '', action="append", dest="warnings", default=[],
>                   "pass a warning")
> 
> If you pass in -Wall -Wsliff-sloff options.warnings will then contain ['all',
> 'sliff-sloff'].

But this is the thing - I don't think there's really a useful separation of warnings for us.  -Wlist-returns?  -Wbare-pointers?  Doesn't seem too interesting.

I'm not saying I can't see it in the future.  Maybe we can call -Wintrospection -Wall for now?

> This should be moved out of transformer.py and be a public function

But currently just the transformer knows what namespace it's processing.  We could potentially put this in each Node I guess...

> giscanner/log:warn(node, string, context) or so.
> 
> Can probably be more flexible, to allow the callsites of this function to do
> most of the string formatting.

Hmm...can you explain what caller would want to format things differently?

Attachment 163703 [details] pushed as 27b3465 - Add some more Gio annotations

I fixed up some more things and pushed, but am sure we are going to be further refining this, and am willing to do more on that.

The main problem as I see it right now is that the way AnnotationParser assigns defaults is a little hacky.  Mostly this is because we don't have a good way to say right now is_node_gobject(node) for example.

I'll probably be working on some transformer patches for this.

I ended up reverting the changeset because it broke the build. I also think there are a few thing that needs to be taken care of before this get pushed. I had another review at it before reverting. It obviously needs a bit more work.

- update the man page
- Use -Wall,-Werror, no double dashes, makes no sense to follow the gcc syntax if it's not exactly the same
- Don't use def func(foo=[]), it's always a bug
- Don't call private methods on foreign objects, eg self._transformer._xxx
- _log_symbol_warnings is unused and should be removed.
- don't use "string" as a variable name as it's a module
- don't use double underscores for private variables
- don't add trailing spaces in girparser.c

The build error we're seeing at the litl buildbot is the following:

Traceback (most recent call last):

+ Trace 222458

(In reply to comment #19)

> 
> The build error we're seeing at the litl buildbot is the following:
> 
> Traceback (most recent call last):

That's the cache.  Every time I get a bizarre python error I've trained myself to try rm -rf ~/.cache/g-ir-scanner

I did add versioning to the cache but there was a bug there which prevented the old files from being cleared out. Should be fixed now, eg, there should never be a reason to manually kill ~/.cache/g-ir-scanner any longer.

(In reply to comment #19)

> - update the man page

Okay...really we should be generating the man page from the optparse data somehow, having all this stuff in two places is rather a fail.

> - Use -Wall,-Werror, no double dashes, makes no sense to follow the gcc syntax
> if it's not exactly the same

I can't, optparse won't let me.  I'd rather pick a different format honestly.

--warn-all --warn-errors?

> - Don't use def func(foo=[]), it's always a bug

I don't see why it's *always* a bug.  You just have to be careful.  But okay, I changed it.

> - Don't call private methods on foreign objects, eg self._transformer._xxx

Okay, I made it public.

> - _log_symbol_warnings is unused and should be removed.

Hm, I was going to use it somewhere but I forgot where now.  I'll look.

> - don't use "string" as a variable name as it's a module

Okay.

> - don't use double underscores for private variables

I changed it.

> - don't add trailing spaces in girparser.c

Fixed.

(In reply to comment #22)
> (In reply to comment #19)
> 
> > - update the man page
> 
> Okay...really we should be generating the man page from the optparse data
> somehow, having all this stuff in two places is rather a fail.

Good idea. Do you want to integrate
http://andialbrecht.wordpress.com/2009/03/17/creating-a-man-page-with-distutils-and-optparse/ (or another variant) into the build?

Guess we still need to maintain the man pages for the tools written in C.

> > - Use -Wall,-Werror, no double dashes, makes no sense to follow the gcc syntax
> > if it's not exactly the same
> 
> I can't, optparse won't let me.  I'd rather pick a different format honestly.
> 

Yes you can, I thought I explained how to do this in a comment, here is a bit clearer code for it:

  parser.add_option('-W', action="append", dest="warnings", default=[])

(In reply to comment #23)
> 
> Yes you can, I thought I explained how to do this in a comment, here is a bit
> clearer code for it:
> 
>   parser.add_option('-W', action="append", dest="warnings", default=[])

But if I do that, then I can't document -Wall and -Werror.

Created attachment 163917 [details] [review]
Support introspectable=no attribute, add warnings framework

This work allows us to move closer to replacing gtk-doc, among other
things.  We add a generic attribute "introspectable", and inside the
typelib compiler if we see "introspectable=no", we don't put it in the
typelib.  This replaces the hackish pre-filter for varargs with a much
more generic mechanism.

The varargs is now handled in the scanner, and we emit
introspectable=no for them.

Add generic metadata to Node with references to file/line/column,
which currently comes from symbols.

Add scanner options --warn-all and --warn-error.

You can probably document it by adding/modifying the help formatter where you can append the -Wxxx options.

I decided to just use --warn-all and --warn-error, it's actually more confusing to follow GCC I think because people might think we pick up CFLAGS etc.

Attachment 163917 [details] pushed as 5a4fa2b - Support introspectable=no attribute, add warnings framework

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]