GNOME Bugzilla – Bug 753520
Gtk potentially misses nullable in 166 functions
Last modified: 2016-01-08 20:18:37 UTC
I have written a script that checks for explicit mentions of "or %NULL" and have discovered a potential list of 166 potential places where the annotations are wrong, I'm using this bug to track the progress of reviewing these "warnings" and fixing the annotations on a wip branch, here's the list for reference: Gtk.AccelGroup.query -> Gtk.AccelGroup.from_accel_closure -> Gtk.AccelLabel.get_accel_widget -> Gtk.Accessible.get_widget -> Gtk.Action.create_menu -> Gtk.Action.get_accel_path -> Gtk.Action.create_menu -> Gtk.ActionGroup.get_accel_group -> Gtk.ActionGroup.get_action -> Gtk.ActionGroup.get_action -> Gtk.Actionable.get_action_name -> Gtk.Actionable.get_action_name -> Gtk.AppChooser.get_app_info -> Gtk.AppChooserButton.get_heading -> Gtk.AppChooserDialog.get_heading -> Gtk.Assistant.get_nth_page -> Gtk.Assistant.get_page_header_image -> Gtk.Assistant.get_page_side_image -> Gtk.Builder.get_application -> Gtk.Builder.get_object -> Gtk.Builder.lookup_callback_symbol -> Gtk.Button.get_image -> Gtk.CalendarDetailFunc -> Gtk.CellArea.get_focus_from_sibling -> Gtk.CellRenderer.start_editing -> Gtk.CellRenderer.start_editing -> Gtk.CellView.get_displayed_row -> Gtk.CellView.get_model -> Gtk.Container.get_focus_child -> Gtk.Container.get_focus_hadjustment -> Gtk.Container.get_focus_vadjustment -> Gtk.Dialog.get_widget_for_response -> Gtk.Entry.get_attributes -> Gtk.Entry.get_cursor_hadjustment -> Gtk.Entry.get_icon_gicon -> Gtk.Entry.get_icon_name -> Gtk.Entry.get_icon_pixbuf -> Gtk.Entry.get_icon_stock -> Gtk.EntryCompletion.compute_prefix -> Gtk.EntryCompletion.get_model -> Gtk.Expander.get_label_widget -> Gtk.FileChooser.get_current_folder -> Gtk.FileChooser.get_extra_widget -> Gtk.FileChooser.get_filename -> Gtk.FileChooser.get_filter -> Gtk.FileChooser.get_preview_file -> Gtk.FileChooser.get_preview_filename -> Gtk.FileChooser.get_preview_uri -> Gtk.FileChooser.get_preview_widget -> Gtk.FileChooser.get_uri -> Gtk.FileFilter.get_name -> Gtk.FontChooser.get_font -> Gtk.FontChooser.get_font_desc -> Gtk.FontChooser.get_font_face -> Gtk.FontChooser.get_font_family -> Gtk.FontChooser.get_font_face -> Gtk.FontChooser.get_font_family -> Gtk.FontSelection.get_font_name -> Gtk.FontSelectionDialog.get_font_name -> Gtk.Frame.get_label -> Gtk.GLArea.get_error -> Gtk.Gesture.get_device -> Gtk.Gesture.get_window -> Gtk.Grid.get_child_at -> Gtk.HeaderBar.get_custom_title -> Gtk.HeaderBar.get_subtitle -> Gtk.HeaderBar.get_title -> Gtk.IconFactory.lookup_default -> Gtk.IconInfo.get_builtin_pixbuf -> Gtk.IconInfo.get_filename -> Gtk.IconView.get_model -> Gtk.IconView.get_path_at_pos -> Gtk.Image.get_animation -> Gtk.Image.get_pixbuf -> Gtk.Label.get_attributes -> Gtk.Label.get_mnemonic_widget -> Gtk.ListBox.get_row_at_index -> Gtk.ListBoxRow.get_header -> Gtk.Menu.get_title -> Gtk.MenuButton.get_align_widget -> Gtk.MenuButton.get_menu_model -> Gtk.MenuButton.get_popover -> Gtk.MenuButton.get_popup -> Gtk.MenuItem.get_accel_path -> Gtk.MenuItem.get_submenu -> Gtk.Notebook.get_action_widget -> Gtk.Notebook.get_group_name -> Gtk.Notebook.get_menu_label -> Gtk.Notebook.get_menu_label_text -> Gtk.Notebook.get_nth_page -> Gtk.Notebook.get_tab_label_text -> Gtk.NumerableIcon.get_background_gicon -> Gtk.NumerableIcon.get_background_icon_name -> Gtk.NumerableIcon.get_style_context -> Gtk.OffscreenWindow.get_pixbuf -> Gtk.OffscreenWindow.get_surface -> Gtk.Paned.get_child1 -> Gtk.Paned.get_child2 -> Gtk.Plug.get_socket_window -> Gtk.ProgressBar.get_text -> Gtk.RecentFilter.get_name -> Gtk.RecentManager.lookup_item -> Gtk.Scale.get_layout -> Gtk.Socket.get_plug_window -> Gtk.StackSwitcher.get_stack -> Gtk.StatusIcon.get_gicon -> Gtk.StatusIcon.get_icon_name -> Gtk.StatusIcon.get_pixbuf -> Gtk.StatusIcon.get_stock -> Gtk.StatusIcon.get_tooltip_markup -> Gtk.StatusIcon.get_tooltip_text -> Gtk.StyleContext.get_frame_clock -> Gtk.StyleContext.get_parent -> Gtk.StyleContext.lookup_icon_set -> Gtk.StyleProvider.get_icon_factory -> Gtk.StyleProvider.get_icon_factory -> Gtk.TextBuffer.get_mark -> Gtk.TextBufferSerializeFunc -> Gtk.TextTagTable.lookup -> Gtk.TextView.get_tabs -> Gtk.TextView.get_window -> Gtk.ThemingEngine.get_screen -> Gtk.ThemingEngine.load -> Gtk.ToolButton.get_icon_name -> Gtk.ToolButton.get_icon_widget -> Gtk.ToolButton.get_label -> Gtk.ToolButton.get_label_widget -> Gtk.ToolPalette.get_drop_group -> Gtk.ToolPalette.get_drop_item -> Gtk.Toolbar.get_nth_item -> Gtk.TreeModelFilter.convert_child_path_to_path -> Gtk.TreeModelFilter.convert_path_to_child_path -> Gtk.TreeModelSort.convert_child_path_to_path -> Gtk.TreeModelSort.convert_path_to_child_path -> Gtk.TreeView.get_bin_window -> Gtk.TreeView.get_column -> Gtk.TreeView.get_hadjustment -> Gtk.TreeView.get_vadjustment -> Gtk.TreeViewColumn.get_widget -> Gtk.UIManager.get_action -> Gtk.UIManager.get_widget -> Gtk.UIManager.get_action -> Gtk.UIManager.get_widget -> Gtk.Widget.drag_dest_get_target_list -> Gtk.Widget.drag_source_get_target_list -> Gtk.Widget.get_ancestor -> Gtk.Widget.get_composite_name -> Gtk.Widget.get_parent -> Gtk.Widget.get_tooltip_markup -> Gtk.Widget.get_tooltip_text -> Gtk.Widget.render_icon -> Gtk.Widget.render_icon_pixbuf -> Gtk.Window.get_application -> Gtk.Window.get_attached_to -> Gtk.Window.get_default_widget -> Gtk.Window.get_focus -> Gtk.Window.get_icon_name -> Gtk.Window.get_role -> Gtk.Window.get_title -> Gtk.Window.get_titlebar -> Gtk.Window.get_transient_for -> Gtk.WindowGroup.get_current_device_grab -> Gtk.get_current_event_device -> Gtk.get_event_widget -> Gtk.grab_get_current -> Gtk.test_find_widget ->
useful!
This new list discards any deprecated class/method, the list is big enough already do worry about deprecated API: Gtk.AccelGroup.query -> Gtk.AccelGroup.from_accel_closure -> Gtk.AccelLabel.get_accel_widget -> Gtk.Accessible.get_widget -> Gtk.Actionable.get_action_name -> Gtk.Actionable.get_action_name -> Gtk.AppChooser.get_app_info -> Gtk.AppChooserButton.get_heading -> Gtk.AppChooserDialog.get_heading -> Gtk.Assistant.get_nth_page -> Gtk.Builder.get_application -> Gtk.Builder.get_object -> Gtk.Builder.lookup_callback_symbol -> Gtk.Button.get_image -> Gtk.CalendarDetailFunc -> Gtk.CellArea.get_focus_from_sibling -> Gtk.CellRenderer.start_editing -> Gtk.CellRenderer.start_editing -> Gtk.CellView.get_displayed_row -> Gtk.CellView.get_model -> Gtk.Container.get_focus_child -> Gtk.Container.get_focus_hadjustment -> Gtk.Container.get_focus_vadjustment -> Gtk.Dialog.get_widget_for_response -> Gtk.Entry.get_attributes -> Gtk.Entry.get_cursor_hadjustment -> Gtk.Entry.get_icon_gicon -> Gtk.Entry.get_icon_name -> Gtk.Entry.get_icon_pixbuf -> Gtk.EntryCompletion.compute_prefix -> Gtk.EntryCompletion.get_model -> Gtk.Expander.get_label_widget -> Gtk.FileChooser.get_current_folder -> Gtk.FileChooser.get_extra_widget -> Gtk.FileChooser.get_filename -> Gtk.FileChooser.get_filter -> Gtk.FileChooser.get_preview_file -> Gtk.FileChooser.get_preview_filename -> Gtk.FileChooser.get_preview_uri -> Gtk.FileChooser.get_preview_widget -> Gtk.FileChooser.get_uri -> Gtk.FileFilter.get_name -> Gtk.FontChooser.get_font -> Gtk.FontChooser.get_font_desc -> Gtk.FontChooser.get_font_face -> Gtk.FontChooser.get_font_family -> Gtk.FontChooser.get_font_face -> Gtk.FontChooser.get_font_family -> Gtk.Frame.get_label -> Gtk.GLArea.get_error -> Gtk.Gesture.get_device -> Gtk.Gesture.get_window -> Gtk.Grid.get_child_at -> Gtk.HeaderBar.get_custom_title -> Gtk.HeaderBar.get_subtitle -> Gtk.HeaderBar.get_title -> Gtk.IconInfo.get_filename -> Gtk.IconView.get_model -> Gtk.IconView.get_path_at_pos -> Gtk.Image.get_animation -> Gtk.Image.get_pixbuf -> Gtk.Label.get_attributes -> Gtk.Label.get_mnemonic_widget -> Gtk.ListBox.get_row_at_index -> Gtk.ListBoxRow.get_header -> Gtk.MenuButton.get_align_widget -> Gtk.MenuButton.get_menu_model -> Gtk.MenuButton.get_popover -> Gtk.MenuButton.get_popup -> Gtk.MenuItem.get_accel_path -> Gtk.MenuItem.get_submenu -> Gtk.Notebook.get_action_widget -> Gtk.Notebook.get_group_name -> Gtk.Notebook.get_menu_label -> Gtk.Notebook.get_menu_label_text -> Gtk.Notebook.get_nth_page -> Gtk.Notebook.get_tab_label_text -> Gtk.OffscreenWindow.get_pixbuf -> Gtk.OffscreenWindow.get_surface -> Gtk.Paned.get_child1 -> Gtk.Paned.get_child2 -> Gtk.Plug.get_socket_window -> Gtk.ProgressBar.get_text -> Gtk.RecentFilter.get_name -> Gtk.RecentManager.lookup_item -> Gtk.Scale.get_layout -> Gtk.Socket.get_plug_window -> Gtk.StackSwitcher.get_stack -> Gtk.StyleContext.get_frame_clock -> Gtk.StyleContext.get_parent -> Gtk.TextBuffer.get_mark -> Gtk.TextBufferSerializeFunc -> Gtk.TextTagTable.lookup -> Gtk.TextView.get_tabs -> Gtk.TextView.get_window -> Gtk.ToolButton.get_icon_name -> Gtk.ToolButton.get_icon_widget -> Gtk.ToolButton.get_label -> Gtk.ToolButton.get_label_widget -> Gtk.ToolPalette.get_drop_group -> Gtk.ToolPalette.get_drop_item -> Gtk.Toolbar.get_nth_item -> Gtk.TreeModelFilter.convert_child_path_to_path -> Gtk.TreeModelFilter.convert_path_to_child_path -> Gtk.TreeModelSort.convert_child_path_to_path -> Gtk.TreeModelSort.convert_path_to_child_path -> Gtk.TreeView.get_bin_window -> Gtk.TreeView.get_column -> Gtk.TreeViewColumn.get_widget -> Gtk.Widget.drag_dest_get_target_list -> Gtk.Widget.drag_source_get_target_list -> Gtk.Widget.get_ancestor -> Gtk.Widget.get_parent -> Gtk.Widget.get_tooltip_markup -> Gtk.Widget.get_tooltip_text -> Gtk.Window.get_application -> Gtk.Window.get_attached_to -> Gtk.Window.get_default_widget -> Gtk.Window.get_focus -> Gtk.Window.get_icon_name -> Gtk.Window.get_role -> Gtk.Window.get_title -> Gtk.Window.get_titlebar -> Gtk.Window.get_transient_for -> Gtk.WindowGroup.get_current_device_grab -> Gtk.get_current_event_device -> Gtk.get_event_widget -> Gtk.grab_get_current -> Gtk.test_find_widget ->
Created attachment 309289 [details] Script to generate the list of candidates This is the script that I am ussing, it uses two regular expressions to try to spot candidates. Attaching it for reference.
I have recently improved the script to spot all the candidates, instead of posting updated versions all the time I've moved things to an editable Google Spreadsheet. My plan is to share this in gtk-devel and try to crowdsource the effort: https://docs.google.com/spreadsheets/d/1okGq07H4NnOqdCRN0KV3QduOQ6zsiomFmqK0pqYNNpo/edit?usp=sharing
I just added a wip branch to track progress publicly: wip/aruiz/nullable-annotations
It looks like the script doesn't take GErrors into account; return values should not be marked nullable if they are only NULL when a GError is set.
Created attachment 309313 [details] Script to generate the list of candidates Well spotted Dan, this new version will attach a ",GError" string. Luckily only gtk_recent_manager_lookup_item is marked as "throws" in the list. This improvement might prove useful for other APIs though, thanks a lot for reviewing.
*** Bug 729763 has been marked as a duplicate of this bug. ***
I'll look over GtkWindow and create a patch fixing any problems I can find (with help from the script of course). If this works well, I'll be happy to take care of more of the parts that don't have an owner in the google doc yet.
Created attachment 315081 [details] [review] Return annotations patch for GtkWindow
Could someone give me feedback on the patch? I would like to know if I did everything right before tackling another file.
Review of attachment 315081 [details] [review]: Thanks! This patch is clearly correct, since it just adds nullable annotations to functions that are documented to return NULL. Do you have a GNOME account or do you want someone to commit this for you?
Well, note the exception in comment #6, but your patch is still fine.
I don't have an account with which I could write to the gtk+ repo, but having it committed wasn't even what I was after right now. I just wanted to get a bit of feedback. Of course if it makes more sense to commit these patched individually as they appear instead of waiting for all of the (nullable) fixes to be done, then I don't have a problem with that. I'll have a look at GtkFileChooser, GtkNotebook, GtkTreeView and GtkWidget (just randomly selected a few) next. Also, about that Google Doc: Does Green in the status column mean there are already patches for those parts of Gtk? And would it be important for my work to be visible there as well, or is everyone involved expected to read all the comments here?
Created attachment 315609 [details] [review] Return annotations patch for GtkFileChooser
Created attachment 315611 [details] [review] Return annotations patch for TreeView
Created attachment 315612 [details] [review] Return annotations patch for GtkWidget I'm not completely sure about this one. It includes the drag-and-drop GtkWidget methods that are defined twice, once in gtkdnd.c, once in gtkdnd-quartz.c. I decided to just copy over the more complete documentation comments from gtkdnd.c to gtkdnd-quartz.c, is that okay?
Created attachment 315613 [details] [review] Return annotations patch for GtkNotebook That's it for today. Still haven't run into any false-positives :)
(In reply to Jonas Platte from comment #17) > I decided to just copy over the more complete documentation comments from > gtkdnd.c to gtkdnd-quartz.c, is that okay? No, that is not ok. There is one place where these functions should be documented, and that is gtkdnd.c
Okay, what's supposed to be in gtkdnd-quartz.c then? Should I add the annotation to the documentation there without changing the rest of it or should there be no doc comment at all?
(In reply to Jonas Platte from comment #20) > Okay, what's supposed to be in gtkdnd-quartz.c then? Should I add the > annotation to the documentation there without changing the rest of it or > should there be no doc comment at all? I would prefer there to be no doc comments at all, but I guess that might lead to problems due to the missing annotations ?
Created attachment 317322 [details] [review] This patch fixes all missing nullables in gtk+ I've taken into account all the previous patches into this final patch rebased against gtk master. I've put extra care to review that none of the functions are returning NULL due to errors. In fact there is one case where I am not sure what to do: GtkTextBufferSerializeFunc This function says this in the docs: Returns: a newly-allocated array of guint8 which contains the serialized data, or NULL if an error occurred This is not a GError, so I am leaning to think that this is indeed nullable and that the programer should check for NULL. However I am not entirely sure how to proceed here so comments are welcome.
Created attachment 317323 [details] [review] This patch fixes all missing nullables in gtk+ I've taken into account all the previous patches into this final patch rebased against gtk master. I've put extra care to review that none of the functions are returning NULL due to errors. In fact there is one case where I am not sure what to do: GtkTextBufferSerializeFunc This function says this in the docs: Returns: a newly-allocated array of guint8 which contains the serialized data, or NULL if an error occurred This is not a GError, so I am leaning to think that this is indeed nullable and that the programer should check for NULL. However I am not entirely sure how to proceed here so comments are welcome.
*** Bug 759391 has been marked as a duplicate of this bug. ***
(In reply to Alberto Ruiz from comment #23) > This is not a GError, so I am leaning to think that this is indeed nullable > and that the programer should check for NULL. However I am not entirely sure > how to proceed here so comments are welcome. Agreed.
So, has this patch been committed now?
No, it is not committed, I need to add the fix for GtkTextBufferSerializeFunc and wait for further reviews from the Gtk+ maintainers.
Created attachment 317983 [details] [review] This patch fixes nullable return values fixes for the following symbols in gtk This is a new patch with the fixes needed for GtkTextBufferSerializeFunc. I guess this patch is ready for someone to review it
Review of attachment 317983 [details] [review]: Not sure what I can say in terms of review here... it looks fine to me. One thing I noticed while looking it over is that we seem to have a mixture of Returns: (transfer none) (nullable): ... and Returns: (nullable) (transfer none): ... Is there a preferred order for these ?
Created attachment 318205 [details] [review] This patch fixes nullable return values fixes for the following symbols in gtk This fixes the annotation order, there's no recommendation but at least I've made them consistent within this patch (nullable) first and (transfer ...) after
Review of attachment 318205 [details] [review]: Ok then. Lets get this committed now so we have some time to look for regressions.
Review of attachment 318205 [details] [review]: Committed to master at 496f0892fcfcd14715b85258f155b65d2ab54ab6
Awesome! Are there bug reports for the same thing in other gobject libraries?
I've opened one with a patch for JSON in https://bugzilla.gnome.org/show_bug.cgi?id=759938