B906: Do not warn for missing `visit` calls for bottom-level nodes

[felix-hilden]

Hey hey!

Just upgraded to the latest version to find some neat warnings (B906) about ast.NodeVisitor functions missing a further visit call. The suggestion was to call generic_visit. This is awesome in general, but unnecessary for:

• Global
• Nonlocal
• Name
• MatchAs
• alias

Might be others as well, but these were the ones I hit. If you think it's reasonable, I'd like to suggest that bugbear suppresses the warnings for these nodes.

byeeeee

[jakkdl]

Glad you like the warning! I found it incredibly frustrating myself whenever I forgot to add one and stuff broke, so I'm happy it's used by others as well.

It actually does try to check for that with

flake8-bugbear/bugbear.py

Lines 1000 to 1006 in 4a0db1a

	# extract what's visited, only error if it's a valid ast subclass
	# with a non-empty _fields attribute - which is what's iterated over in
	# ast.NodeVisitor.generic_visit
	class_name = node.name[len("visit_") :]
	class_type = getattr(ast, class_name, None)
	if class_type is None or not getattr(class_type, "_fields", None):
	return

which e.g. stops it from warning on Continue and Break

Onto your list:

• Global and Nonlocal only has a names field, which looks like it can only be strings and not an ast class. So these could be filtered out
• Name has a field ctx which is an ast.Load, so in theory one could have a visit_Load function where it would matter.
• MatchAs looks to be able to contain a MatchSequence which can contain another MatchAs - so this one should definitely not be ignored.
• alias has name and asname, both which seems to only be string/None and can be filtered.

I'm unfortunately not aware of any way to check what possible types an ast node field can be, and looks like you can't just look at the values in _fields either ('names' in Global/Nonlocal can only be strings, but in ImportFrom they can be aliases), so would probably have to be an explicit list of nodes.

But overall I don't think it's a bad habit to have generic_visit in visit_ methods even when they're not strictly necessary, it means developers don't have to keep track of which nodes are end nodes, and that not having one is an explicit choice to show that you don't want to iterate further. It also makes it more robust against the ast changing between different python versions.

[felix-hilden]

Oh, thank you for the clarifications and sorry for not digging in deeper. Major brain fart with the MatchAs 😅 In any case, even including Global, Nonlocal and alias via an explicit list would be really nice 👍 but I also do appreciate the habit and lesser cognitive burden of just calling generic_visit on everything. Up to you, and feel free to close the issue if you think it's concluded!

[jakkdl]

Not much reason not to exclude them, given that pass/break/continue are excluded already. Was reasonably easy to find them all by scanning through the abstract grammar on https://docs.python.org/3/library/ast.html#abstract-grammar, so added a couple more ones - though might have missed one or two.