feat(lsp): extract real Ty from NetType field specs (fixes TODO(type-hints)) - #1276
Dev-X25874 wants to merge 3 commits into
Conversation
Replace Ty::any() at both TODO(type-hints) sites in NetTypeGen: - param_spec(): constructor params now carry concrete Ty for signature-help - eval_type(): known_fields now carry concrete Ty for hover/completion New helper ty_from_field_spec() handles FieldGen<V>, bare NetType, and falls back to Ty::any() for unrecognised spec kinds.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| // Safe fallback — lets the LSP accept any value rather than emit a wrong type. | ||
| Ty::any() |
There was a problem hiding this comment.
🟡 Bare field types remain untyped
ty_from_field_spec returns any for supported bare specs such as str, enums, Symbol, and physical types. Their constructor parameters and instance fields retain inaccurate hints.
Learn more
Net types accept both field(type, ...) wrappers and direct type specifications. The direct path is validated through TypeCompiled::new in NetType::new, and existing uses include primitive, enum, symbol, and physical-value constructors. The helper recognizes only wrapped fields and net constructors, so every other valid direct specification reaches Ty::any().
Example: Signal = builtin.net_type("Signal", frequency=int) validates frequency as an integer at runtime. The LSP still displays frequency: any in Signal(...) and on a Signal instance instead of frequency: int.
Recommended fix: Add a general extraction path for valid direct type specifications, using the Starlark value's instance/evaluation type or the same compiled type representation used during validation. Preserve Ty::any() only when no concrete instance type can be derived, and add coverage for primitive, enum, Symbol, and physical-value specs.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4cca810. Configure here.
| ArcStr::from(field_name.as_str()), | ||
| ParamIsRequired::No, | ||
| Ty::any(), | ||
| ty_from_field_spec(field_spec.to_value()), |
There was a problem hiding this comment.
Constructor types reject implicit conversions
High Severity
param_spec now uses the stored field Ty as the constructor argument type. Runtime still accepts implicit conversions such as strings and scalars for physical fields like voltage. With static typechecking enabled, common calls such as Power(voltage="3.3V") can fail even though they evaluate correctly.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4cca810. Configure here.
1 participant
Resolves both TODO(type-hints) markers in crates/pcb-zen-core/src/lang/net.rs.
Single file changed. New ty_from_field_spec() helper replaces Ty::any() in param_spec() and eval_type() so LSP clients see concrete types instead of any.
Note
Low Risk
Changes only static typing hints for LSP/typechecker; runtime field validation still goes through existing
compile_field_type/validate_fieldpaths with a safeTy::any()fallback.Overview
Improves LSP and typechecker metadata for
NetTypeconstructors and net instances by emitting concrete field types instead ofTy::any().Adds
ty_from_field_spec()as the shared extractor: it reads types fromfield(...)specs (FieldGen+TypeCompiled), from bare net type constructors viaeval_type(), and falls back toTy::any()when the spec shape is unknown.param_spec()now attaches those types to each optional keyword field (constructor signature help / hover).eval_type()uses the same helper forknown_fieldson custom net instance types (field hover and completion). The twoTODO(type-hints)comments in this file are removed.Reviewed by Cursor Bugbot for commit cb71242. Bugbot is set up for automated code reviews on this repo. Configure here.