[Misc] Fix purity checker to allow accessing constants from quadrants modules

[hughperkins]

The purity checker was special-casing qd.math via a local import. Replace with a general is_from_quadrants_module() check that covers any quadrants module/class/instance, and remove the qd_math special case.

Add tests for both the allowed case (qd.math.pi) and the rejected case (non-quadrants object attribute).

Issue: #

Brief Summary

What this PR does:
The purity checker in build_Attribute previously special-cased qd.math (via a local from quadrants import math as qd_math import) to suppress false-positive PURE.VIOLATION
warnings when pure kernels accessed constants like qd.math.pi. This PR replaces that narrow special-case with a general is_from_quadrants_module() check that allows accessing
attributes on any quadrants module or class without triggering purity violations. This is needed for things like qd.simt.Tile16x16.SIZE in the tiles work.
Changes:
• util.py: Add is_from_quadrants_module() — returns True for quadrants modules and classes, but not instances
• ast_transformer.py: Replace the qd_math special-case with is_from_quadrants_module(), remove the now-unused qd_math import
• test_pure_validation.py: Add two tests — one verifying quadrants module constants are allowed (qd.math.pi), one verifying non-quadrants object attributes still warn

Good:
• Generalizes correctly: any quadrants module/class constant (e.g. qd.simt.Tile16x16.SIZE, qd.math.pi) is allowed without needing per-module special cases
• Restricted to modules and classes — instances are still flagged, so mutable runtime state can't silently bypass purity
• Exact package name matching (== "quadrants" or .startswith("quadrants.")) avoids false matches on hypothetical third-party packages
• Removes dead-weight qd_math import that ran on every build_Attribute call
• Compile-time only, zero runtime performance impact

Bad / caveats:
• Mutable class-level attributes (e.g. SomeQdClass.counter = 0 modified at runtime) would be incorrectly suppressed. This is unlikely in practice but not impossible.
• The qd.math.pi test overlaps with the existing test_pure_validation_builtin_values_qd_pi test — though that test was previously covered by the qd_math special-case, not by
is_from_quadrants_module, so it does exercise a different code path now.

copilot:summary

Walkthrough

copilot:walkthrough

[hughperkins]

@claude review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6b32c7f07b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Limit purity bypass to immutable quadrants constants

The new _is_from_quadrants_module(node.value.ptr) guard suppresses pure-kernel violations for any global object defined under the quadrants package, not just safe module constants. In build_Attribute, this now allows reads like cfg = qd.lang.impl.current_cfg(); return cfg.arch to pass without warning/error even though cfg is mutable runtime state that can change between qd.init() calls, which undermines purity and can make fast-cache results stale or incorrect. The exemption should stay scoped to known immutable modules/constants (as before with qd.math) or include an immutability check.

Useful? React with 👍 / 👎.

[hughperkins]

Check codex point

### question to opus

claude says "The new _is_from_quadrants_module(node.value.ptr) guard suppresses pure-kernel violations for any global object defined under the quadrants package, not just
safe module constants. In build_Attribute, this now allows reads like cfg = qd.lang.impl.current_cfg(); return cfg.arch to pass without warning/error even though cfg is
mutable runtime state that can change between qd.init() calls, which undermines purity and can make fast-cache results stale or incorrect. The exemption should stay scoped
to known immutable modules/constants (as before with qd.math) or include an immutability check." Thoguhts?

opus response

  Valid concern, but I think the practical risk is low. Let me explain why.
  The purity violation only fires when all of these hold:
  1. Kernel is @qd.pure
  2. The attribute chain started from a non-parameter global (violates_pure)
  3. The resolved value is isinstance(node.ptr, (int, float, Field))
  4. It's not an enum, not math/np

  The Claude example (cfg = qd.lang.impl.current_cfg(); return cfg.arch) — cfg.arch is likely an Arch enum, which is already exempted by the isinstance(node.ptr, enum.Enum) check
  above.
  That said, the concern is theoretically valid: a mutable int attribute on a quadrants instance (e.g. some_qd_obj.count = 5) would be incorrectly suppressed. The old tiles-3
  check (_is_quadrants_internal_file) was narrower — it only suppressed when the calling code was inside quadrants, not when the object was from quadrants.
  Two options:
  1. Keep as-is — pragmatic. The realistic use case is module/class constants like Tile16x16.SIZE, qd.math.pi. Nobody is putting mutable int attributes on quadrants objects and
     accessing them from pure kernels.
  2. Restrict to modules and classes only — tighten is_from_quadrants_module to reject instances:


  def is_from_quadrants_module(obj: object) -> bool:
      import types
      if isinstance(obj, types.ModuleType):
          return getattr(obj, "__name__", "").startswith("quadrants")
      if isinstance(obj, type):
          mod = getattr(obj, "__module__", None)
          return mod is not None and mod.startswith("quadrants")
      return False

  This would allow qd.math.pi (module attr) and Tile16x16.SIZE (class attr) but reject some_instance.value. That's probably the right tradeoff. Want me to tighten it?

yes

  Done. Pushed 7ef2c4578 — is_from_quadrants_module now only returns True for modules and classes, not instances.

[hughperkins]

@claude review

[hughperkins]

@claude review

[claude]

🟡 The new test_pure_validation_quadrants_module_attribute_allowed test (line 257) is functionally identical to the pre-existing test_pure_validation_builtin_values_qd_pi (line 175): both define a @qd.kernel(pure=True) that returns qd.math.pi and assert int(k1() * 100) == 314. This duplicate adds zero new coverage. The isinstance(obj, type) (class-attribute) branch introduced in is_from_quadrants_module() remains completely untested — a test accessing a constant on a quadrants class type (e.g. SomeQuadrantsClass.CONSTANT) would provide meaningful coverage of the new code path.

Extended reasoning...

Duplicate test with no additional coverage

The newly added test_pure_validation_quadrants_module_attribute_allowed (lines 257-264) replicates test_pure_validation_builtin_values_qd_pi (lines 175-188) nearly verbatim. Both kernels close over qd.math.pi, both assert int(k1() * 100) == 314, and both exercise exactly the same ModuleType branch in is_from_quadrants_module(). The only cosmetic differences in the older test are an unused MyEnum class declaration and an extra assignment v = k1() — neither affects what code path is exercised.

The specific code path that is (and is not) covered

is_from_quadrants_module() in python/quadrants/lang/util.py has two branches:

1. isinstance(obj, types.ModuleType) → returns True when obj.__name__ starts with "quadrants". Both tests exercise this branch via qd.math (a module).
2. isinstance(obj, type) → returns True when obj.__module__ starts with "quadrants". No test exercises this branch.

The class-attribute path was explicitly added to allow patterns like Tile16x16.SIZE (accessing a constant on a quadrants class type), but the test suite never exercises it.

Step-by-step proof of duplication

1. test_pure_validation_builtin_values_qd_pi: defines def k1() -> qd.f32: return qd.math.pi, calls it, asserts int(k1() * 100) == 314.
2. test_pure_validation_quadrants_module_attribute_allowed: defines the same kernel body, calls it, makes the same assertion.
3. In both cases node.value.ptr in build_Attribute is the qd.math module object, hitting the ModuleType branch of is_from_quadrants_module().
4. The isinstance(obj, type) branch remains dead code in the test suite.

Impact

This is a test-quality issue only — no production code is broken. The duplicate test is harmless, but the missing class-attribute coverage means a regression in that branch could go undetected.

How to fix

Remove or repurpose the duplicate test. A meaningful replacement would access a float/int constant on a quadrants class type directly, for example:

@qd.kernel(pure=True)
def k() -> qd.f32:
    return SomeQuadrantsClass.CONSTANT  # triggers isinstance(obj, type) branch

This would exercise the class branch and provide coverage for the new logic introduced in this PR.

[claude]

The change from pure=True to fastcache=True does not address the core concern. Both test_pure_validation_quadrants_module_attribute_allowed and the pre-existing test_pure_validation_builtin_values_qd_pi still access qd.math.pi (a ModuleType attribute), so they continue to exercise only the isinstance(obj, types.ModuleType) branch of is_from_quadrants_module(). The isinstance(obj, type) branch — which enables the stated primary motivation of this PR (Tile16x16.SIZE access) — remains completely untested.

A meaningful test would access a float/int constant on an actual quadrants class type, for example using a type from the quadrants package whose __module__ starts with "quadrants.", to confirm that _is_from_quadrants_module returns True for the class branch and the violation is suppressed.

[claude]

🔴 The purity exemption in build_Attribute clears the violation flag for all three types in the outer isinstance(node.ptr, (int, float, Field)) check when _is_from_quadrants_module(node.value.ptr) returns True — including Field. If a quadrants class ever has a class-level Field attribute (e.g. ConstantFields.my_field), accessing it inside a pure=True kernel silently suppresses the violation. Fix by adding and not isinstance(node.ptr, Field) to the exemption guard on line 683.

Extended reasoning...

What the bug is

In build_Attribute (lines 677–685), the outer guard checks isinstance(node.ptr, (int, float, Field)) and sets violation = True. Further down, the new _is_from_quadrants_module exemption unconditionally clears violation for all three matched types — including Field. Field objects represent mutable device memory and should never bypass the purity checker, regardless of which module their owning class comes from.

The code path that triggers it

is_from_quadrants_module returns True for any type whose __module__ attribute starts with "quadrants". This means any quadrants-internal class (e.g. one defined in quadrants.lang.something) satisfies the check. If such a class has a class-level Field attribute, the sequence in build_Attribute is:

1. node.ptr = ConstantFields.my_field (a Field instance)
2. isinstance(node.ptr, (int, float, Field)) → True, violation = True
3. isinstance(node.ptr, enum.Enum) → False
4. node.value.ptr in [math, np] → False
5. _is_from_quadrants_module(ConstantFields) → True (it is a type with __module__ = "quadrants.lang.something")
6. violation = False — incorrectly suppressed

Why existing code does not prevent it

The isinstance(obj, type) branch in is_from_quadrants_module was deliberately added to support patterns like Tile16x16.SIZE (an int constant on a quadrants class). This is correct for int/float class attributes, but the same logic extends to Field attributes without any additional guard. There is no separate check that restricts the exemption to immutable scalar types only.

Impact

A quadrants class-level Field attribute accessed inside a pure=True kernel would silently pass the purity checker. Field objects are mutable device state; their values can change between qd.init() calls and across kernel invocations, which is precisely what the purity contract is designed to prohibit. Stale fast-cache results could be returned for kernels that appear pure but actually depend on mutable fields.

Step-by-step proof

Suppose a file inside the quadrants package defines:

# in quadrants/lang/something.py
class ConstantFields:
    my_field = qd.field(qd.f32, shape=None)

User code:

@qd.kernel(pure=True)
def k() -> qd.f32:
    return ConstantFields.my_field[0]

During compilation, build_Attribute resolves ConstantFields.my_field:

• node.value.ptr = ConstantFields (a type with __module__ = "quadrants.lang.something")
• node.ptr = the Field instance
• isinstance(node.ptr, (int, float, Field)) → True, violation = True
• _is_from_quadrants_module(ConstantFields) → True
• violation = False — no warning, no error, purity violation silently dropped

How to fix

Change line 683 from:

if violation and _is_from_quadrants_module(node.value.ptr):

to:

if violation and _is_from_quadrants_module(node.value.ptr) and not isinstance(node.ptr, Field):

This preserves the exemption for int/float class constants (the intended use case for Tile16x16.SIZE, qd.math.pi, etc.) while keeping Field access subject to purity checking regardless of which package the owning class comes from. No current quadrants class has a class-level Field attribute, so this is low-risk but worth fixing before the pattern becomes established.

[claude]

🟡 The new test test_pure_validation_non_quadrants_attribute_warns (lines 267-284) duplicates the existing second sub-test in test_pure_validation_actual_violation_warning (lines 238-251): both access an uppercase int attribute on a non-quadrants class instance from a pure kernel and expect a PURE.VIOLATION UserWarning. The new test adds no unique coverage; more valuable would be a test verifying that a quadrants CLASS attribute (e.g., SomeQdClass.CONSTANT) is correctly allowed — which is the key invariant introduced in this PR but left completely untested.

Extended reasoning...

What the duplication is

Both test_pure_validation_non_quadrants_attribute_warns (new, cfg.SIZE = 32, uppercase class attribute on a non-quadrants instance) and the second sub-test of test_pure_validation_actual_violation_warning (existing, foo.BAR = 23, uppercase instance attribute on a non-quadrants instance) exercise the identical code path in build_Attribute. In both cases: node.value.ptr is a plain Python instance (not a module, not a type), _is_from_quadrants_module(node.value.ptr) returns False (the function explicitly returns False for anything that is neither ModuleType nor type), node.ptr is an int, and the uppercase attribute name triggers warnings.warn.

Why the class-vs-instance-attribute distinction does not matter

The new test uses a class-level attribute (Config.SIZE = 32) while the existing test uses an instance-level attribute (self.BAR = 23). However, at the point build_Attribute runs, Python has already resolved both to the same int value via getattr(cfg, 'SIZE') and getattr(foo, 'BAR') respectively. The AST transformer sees an identical picture in both cases: node.ptr is an int, node.value.ptr is a non-quadrants instance. The code path is byte-for-byte the same.

Why this matters despite being a test-quality issue only

The PR's stated goal is to generalize purity exemptions from qd.math (module) to any quadrants module or class. The isinstance(obj, type) branch in is_from_quadrants_module() — which enables accessing constants like Tile16x16.SIZE — has zero test coverage. The two new tests both hit the ModuleType branch (qd.math.pi) and the non-quadrants-instance path (duplicate of existing), leaving the class-attribute branch (the primary motivation for the PR) completely untested.

The refutation addressed

The one refutation argues only that "having a redundant test doesn't cause harm" and confirms severity is nit. This is accurate — no production code is affected. The concern is purely test quality: the new test should instead exercise the class-attribute branch of is_from_quadrants_module to provide meaningful regression coverage for the new logic.

Step-by-step proof of duplication

1. Existing test: foo = Foo() where Foo defines self.BAR = 23. Kernel: return foo.BAR. In build_Attribute: node.value.ptr = foo (instance), node.ptr = 23 (int), isinstance(23, (int, float, Field)) → True, _is_from_quadrants_module(foo) → False (not ModuleType, not type), 'BAR'.upper() == 'BAR' → True → warnings.warn.
2. New test: cfg = Config() where Config defines SIZE = 32. Kernel: return cfg.SIZE. In build_Attribute: node.value.ptr = cfg (instance), node.ptr = 32 (int), isinstance(32, (int, float, Field)) → True, _is_from_quadrants_module(cfg) → False (not ModuleType, not type), 'SIZE'.upper() == 'SIZE' → True → warnings.warn.
3. The two paths are identical. A test accessing SomeQuadrantsClass.CONSTANT (where type(SomeQuadrantsClass).__module__.startswith('quadrants')) would instead exercise the isinstance(obj, type) branch and provide real coverage.

[erizmr]

Approving on the basis that I have reviewed the design and the public facing API, and they look reasonable to me.

[claude]

🔴 The PR's stated primary motivation — allowing qd.simt.Tile16x16.SIZE in pure kernels — is not actually fixed: Tile16x16 is a StructType instance (returned by qd.dataclass()), which inherits from CompoundType(object) rather than Python's built-in type, so isinstance(Tile16x16, type) is False and is_from_quadrants_module returns False for it. Accessing Tile16x16.SIZE in a pure=True kernel still emits a PURE.VIOLATION warning after this PR merges. A third branch in is_from_quadrants_module is needed to check type(obj).__module__ for objects that are neither ModuleType nor type.

Extended reasoning...

What the bug is and how it manifests

is_from_quadrants_module() in util.py:377-383 has exactly two branches: one for types.ModuleType (covers qd.math, qd.simt, etc.) and one for type (covers Python metaclass-style classes like plain class Foo). It returns False for any object that is neither. StructType — the type returned by qd.dataclass(_Tile16x16) — inherits from CompoundType, which inherits from plain object. It does not inherit from Python's type. Therefore isinstance(Tile16x16, type) is False, and is_from_quadrants_module(Tile16x16) falls through to return False.

The specific code path that triggers it

In build_Attribute (ast_transformer.py:683), when a pure kernel accesses an attribute, the purity exemption check is:

if violation and _is_from_quadrants_module(node.value.ptr):
    violation = False

For qd.simt.Tile16x16.SIZE, node.value.ptr is the StructType instance. Since _is_from_quadrants_module returns False, violation stays True and warnings.warn fires with a PURE.VIOLATION message.

Why existing code doesn't prevent it

The fix intentionally avoids returning True for arbitrary instances to prevent mutable quadrants objects from bypassing purity checks — a correct design decision. However, StructType objects returned by qd.dataclass() are effectively class-like constants (created at module load time, not mutated at runtime), and they are the exact objects that serve as namespaces for constants like SIZE = 16. The isinstance(obj, type) check only matches objects that are themselves Python metaclass instances, which StructType is not.

Step-by-step proof

1. _tile16.py:468-469 shows: result = qd.dataclass(_Tile16x16) then result.SIZE = _TILE (= 16). result is a StructType instance.
2. User writes @qd.kernel(pure=True) def k() -> qd.i32: return qd.simt.Tile16x16.SIZE.
3. build_Attribute resolves Tile16x16.SIZE: node.value.ptr = Tile16x16 (StructType), node.ptr = 16 (int).
4. isinstance(16, (int, float, Field)) → True, so violation = True.
5. isinstance(16, enum.Enum) → False.
6. node.value.ptr in [math, np] → False.
7. _is_from_quadrants_module(Tile16x16): isinstance(Tile16x16, types.ModuleType) → False; isinstance(Tile16x16, type) → False (StructType inherits from CompoundType(object), not type); returns False.
8. violation remains True → warnings.warn fires: PURE.VIOLATION.

Impact

The PR description explicitly states: "This is needed for things like qd.simt.Tile16x16.SIZE in the tiles work" and "any quadrants module/class constant (e.g. qd.simt.Tile16x16.SIZE, qd.math.pi) is allowed". Neither statement is true for StructType instances after this PR. Any tiles code accessing Tile16x16.SIZE in a pure kernel will continue to get spurious PURE.VIOLATION warnings. The fix only works for Python modules (qd.math) and actual Python classes (isinstance(obj, type) is True), leaving the stated primary motivation unaddressed.

How to fix it

Add a third branch to is_from_quadrants_module that checks the runtime type's module for objects that are neither ModuleType nor type:

instance_mod = getattr(type(obj), '__module__', None)
if instance_mod is not None and (instance_mod == 'quadrants' or instance_mod.startswith('quadrants.')):
    return True

This should be done carefully: the PR description notes mutable instance attributes should still be flagged, so this branch should arguably only cover known-immutable patterns. At minimum, StructType instances (whose type's __module__ is quadrants.lang.struct) need to be covered for the stated use case to actually work.

[claude]

🟡 The PR introduces a second separate from quadrants.lang.util import (...) block (lines 43–45) immediately before the existing one (lines 46–48), violating the consolidation convention established by commit d106844 ('Consolidate util imports into a single import block'). Both imports should be merged into a single block.

Extended reasoning...

What the bug is and how it manifests

The PR adds is_from_quadrants_module as _is_from_quadrants_module as its own from quadrants.lang.util import (...) block (lines 43–45) directly above the pre-existing from quadrants.lang.util import (is_quadrants_internal_file as _is_quadrants_internal_file,) block (lines 46–48). The result is two consecutive import blocks from the same module, which is redundant and inconsistent with the project's established style.

The specific code path that triggers it

The import section of ast_transformer.py now reads:

from quadrants.lang.util import (
    is_from_quadrants_module as _is_from_quadrants_module,
)
from quadrants.lang.util import (
    is_quadrants_internal_file as _is_quadrants_internal_file,
)

Both names come from the same module and should live in a single block.

Why existing tooling doesn't prevent it

The refutation notes that ruff was run immediately after (commit 73e5098, 'style: fix ruff import sorting in ast_transformer') and did not flag this. This is expected: ruff's default isort configuration does not automatically merge separate import blocks from the same module unless force-sort-within-sections or equivalent settings are enabled. The ruff pass fixed ordering within blocks but left the split itself intact. This means the project convention (established explicitly by d106844) is stricter than the current ruff configuration enforces.

What the impact would be

No functional impact — both names are correctly imported and used. This is a purely cosmetic style violation that makes the import section harder to scan and sets a precedent for further split imports as the file grows.

How to fix it

Merge the two blocks into one:

from quadrants.lang.util import (
    is_from_quadrants_module as _is_from_quadrants_module,
    is_quadrants_internal_file as _is_quadrants_internal_file,
)

Step-by-step proof

1. Commit d106844 explicitly titled 'Consolidate util imports into a single import block' establishes the convention.
2. The PR diff shows two from quadrants.lang.util import (...) blocks added/retained consecutively at lines 43–48.
3. ruff (commit 73e5098) ran and did not merge them — confirming ruff's default config does not enforce this pattern.
4. The correct fix is to manually merge them, as was done in d106844.