Skip to content

ENT-13841: Fixed linting not working with namespaced bundle/body #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
olehermanse merged 1 commit into from
Apr 1, 2026
Merged

ENT-13841: Fixed linting not working with namespaced bundle/body #41

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 95 additions & 31 deletions src/cfengine_cli/lint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,32 +32,59 @@ class _State:
block_type: str | None = None # "bundle" | "body" | "promise" | None
promise_type: str | None = None # "vars" | "files" | "classes" | ... | None
attribute_name: str | None = None # "if" | "string" | "slist" | ... | None
namespace: str = "default" # "ns" | "default" | ... |

def update(self, node) -> "_State":
def update(self, node):
"""Updates and returns the state that should apply to the children of `node`."""
if node.type == "}":
assert node.parent
assert node.parent.type in [
"bundle_block_body",
"promise_block_body",
"body_block_body",
"list",
]
if node.parent.type != "list":
# We just ended a block
self.block_type = None
self.promise_type = None
self.attribute_name = None
return
if node.type == ";":
self.attribute_name = None
return
if node.type == "bundle_block":
return _State(block_type="bundle")
self.block_type = "bundle"
return
if node.type == "body_block":
return _State(block_type="body")
self.block_type = "body"
return
if node.type == "promise_block":
return _State(block_type="promise")
self.block_type = "promise"
return
if node.type == "bundle_section":
for child in node.children:
if child.type == "promise_guard":
return _State(
block_type=self.block_type,
promise_type=_text(child)[:-1], # strip trailing ':'
)
return _State(block_type=self.block_type)
# A bundle_section is always: promise_guard, [promises], [class_guarded_promises...]
# The promise_guard is guaranteed to exist by the grammar
guard = next((c for c in node.children if c.type == "promise_guard"), None)
Copy link
Copy Markdown
Member

@olehermanse olehermanse Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimonThalvorsen Why is this this necessary, isn't promise_guard always node.children[0] ?

Suggested change
guard = next((c for c in node.children if c.type == "promise_guard"), None)
assert node.children
guard = node.children[0]

if guard is None: # Should never happen
print("ERROR: Bundle section without a promise guard")
return
Comment on lines +69 to +71
Copy link
Copy Markdown
Member

@olehermanse olehermanse Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if guard is None: # Should never happen
print("ERROR: Bundle section without a promise guard")
return
assert guard


self.promise_type = _text(guard)[:-1] # strip trailing ':'
return
if node.type == "attribute":
for child in node.children:
if child.type == "attribute_name":
return _State(
block_type=self.block_type,
promise_type=self.promise_type,
attribute_name=_text(child),
)
return self
self.attribute_name = _text(child)
if self.attribute_name == "namespace":
self.namespace = _text(child.next_named_sibling).strip("\"'")
Comment on lines +78 to +80
Copy link
Copy Markdown
Member

@olehermanse olehermanse Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see 2 problems with this:

  1. We don't want to do this whenever attribute name is namespace. We want to do it only inside body file control. So there should be another condition in the if to see if we are inside the right place.
  2. Why use named sibling when we've set up the perfect state tracker to solve this kind of check? IMO we should set this when we get to the rval string (and check what block and attribute we are inside with an appropriate if check).

return
return

@staticmethod
def qualify(name: str, namespace: str) -> str:
"""If name is already qualified (contains ':'), return as-is. Otherwise prepend namespace."""
return name if ":" in name else f"{namespace}:{name}"
Comment on lines +84 to +87
Copy link
Copy Markdown
Member

@olehermanse olehermanse Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No big deal, but I would probably not make this a staticmethod. Can just be a top level function. Maybe prefixed with underscore to make it "internal".



def lint_cfbs_json(filename) -> int:
Expand Down Expand Up @@ -184,7 +211,8 @@ def _node_checks(filename, lines, node, user_definition, strict, state: _State):
if node.type == "calling_identifier":
if (
strict
and _text(node) in user_definition.get("all_bundle_names", set())
and state.qualify(_text(node), state.namespace)
in user_definition.get("all_bundle_names", set())
and state.promise_type in user_definition.get("custom_promise_types", set())
):
_highlight_range(node, lines)
Expand All @@ -193,11 +221,12 @@ def _node_checks(filename, lines, node, user_definition, strict, state: _State):
)
return 1
if strict and (
_text(node)
not in BUILTIN_FUNCTIONS.union(
state.qualify(_text(node), state.namespace)
not in set.union(
user_definition.get("all_bundle_names", set()),
user_definition.get("all_body_names", set()),
)
and _text(node) not in BUILTIN_FUNCTIONS
):
_highlight_range(node, lines)
print(
Expand All @@ -215,11 +244,9 @@ def _stateful_walk(

errors = _node_checks(filename, lines, node, user_definition, strict, state)

child_state = state.update(node)
state.update(node)
for child in node.children:
errors += _stateful_walk(
filename, lines, child, user_definition, strict, child_state
)
errors += _stateful_walk(filename, lines, child, user_definition, strict, state)
return errors


Expand All @@ -239,18 +266,55 @@ def _walk(filename, lines, node, user_definition=None, strict=True) -> int:
line = node.range.start_point[0] + 1
column = node.range.start_point[1] + 1

return _stateful_walk(filename, lines, node, user_definition, strict)
state = _State()
ret = _stateful_walk(filename, lines, node, user_definition, strict, state=state)
state = _State() # Clear state
return ret
Comment on lines +269 to +272
Copy link
Copy Markdown
Member

@olehermanse olehermanse Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear state for what? Isn't this exactly the same as:

Suggested change
state = _State()
ret = _stateful_walk(filename, lines, node, user_definition, strict, state=state)
state = _State() # Clear state
return ret
return _stateful_walk(filename, lines, node, user_definition, strict, state=_State())



def _parse_user_definition(filename, lines, root_node):
promise_blocks = _find_node_type(filename, lines, root_node, "promise_block_name")
bundle_blocks = _find_node_type(filename, lines, root_node, "bundle_block_name")
body_blocks = _find_node_type(filename, lines, root_node, "body_block_name")
ns = "default"
promise_blocks = set()
bundle_blocks = set()
body_blocks = set()

for child in root_node.children:
if child.type == "body_block":
name_node = next(
(c for c in child.named_children if c.type == "body_block_name"),
None,
)
ns_attr = next(
(
c
for c in _find_node_type(filename, lines, child, "attribute_name")
if _text(c) == "namespace"
),
None,
)
if ns_attr is not None:
ns = _text(ns_attr.next_named_sibling).strip("\"'")
Copy link
Copy Markdown
Member

@olehermanse olehermanse Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the same problem as your other logic for detecting namespace - we only want to change namespace using a body file control block, not any attribute or any body.

elif name_node is not None:
body_blocks.add(_State.qualify(_text(name_node), ns))
elif child.type == "bundle_block":
name_node = next(
(c for c in child.named_children if c.type == "bundle_block_name"),
None,
)
if name_node is not None:
bundle_blocks.add(_State.qualify(_text(name_node), ns))
elif child.type == "promise_block":
name_node = next(
(c for c in child.named_children if c.type == "promise_block_name"),
None,
)
if name_node is not None:
promise_blocks.add(_text(name_node))
Copy link
Copy Markdown
Member

@olehermanse olehermanse Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work. I think you need to change your approach to reuse the same code across both passes. At a high level we need to:

  1. Parse all the files and convert them into syntax trees. This step will error on syntax errors.
  2. Loop over all the syntax trees and "discover" definitions. This step should not error (actually might in the future, if we want to add detection for duplicate definitions, for example).
  3. Loop over all the syntax trees again and actually perform our checks. This is where we will error for all the linting rules.

Step 2 and 3 should both use the _stateful_walk function IMO. It can have an argument for mode - for example "discovery" | "linting".

I understand the desire to do step 2 in a simple way as you have it here (just find and collect all the bundles and bodies). However, I don't think it's that simple, you need to have the same / similar context available as in step 3. You need to know which namespace you are in. In the future, if we do something similar for classes and variables, you need to know which bundle you are in, etc.

Another thing we have not handled at all yet is macros - the state could keep track of which macros we are "inside" (@if minimum_version for example).


return {
"custom_promise_types": {_text(x) for x in promise_blocks},
"all_bundle_names": {_text(x) for x in bundle_blocks},
"all_body_names": {_text(x) for x in body_blocks},
"custom_promise_types": promise_blocks,
"all_bundle_names": bundle_blocks,
"all_body_names": body_blocks,
}


Expand Down
Loading