Skip to content

Require a range constraint in ConstrainedScalarSubtypeSymbol - #172

Merged
Paebbels merged 2 commits into
devfrom
claude/mandatory-scalar-constraint
Aug 4, 2026
Merged

Require a range constraint in ConstrainedScalarSubtypeSymbol#172
Paebbels merged 2 commits into
devfrom
claude/mandatory-scalar-constraint

Conversation

@Paebbels

@Paebbels Paebbels commented Aug 4, 2026

Copy link
Copy Markdown
Member

Changes

ConstrainedScalarSubtypeSymbol's range constraint is now mandatory.

VHDL's grammar leaves no state in which this class has no constraint:

subtype_indication ::= [ resolution_indication ] type_mark [ constraint ]
constraint         ::= range_constraint | array_constraint | record_constraint
range_constraint   ::= **range** range
range              ::= range_attribute_name | simple_expression direction simple_expression

The range keyword is always followed by a range, so a subtype indication is either a bare type mark — modelled as SimpleSubtypeSymbol — or a type mark with a range. There is integer and there is integer range 0 to 5, but no integer range.

  • constraint loses its None default and is typed Range instead of Nullable[Range], in ScalarConstraint as well as in ConstrainedScalarSubtypeSymbol.
  • The Constraint property returns Range.

Documentation

  • ScalarConstraint states why the range is mandatory, citing the range_constraint rule.
  • ConstrainedScalarSubtypeSymbol gained a second example for the range-attribute form, subtype index is natural range vector'range;, which is a RangeFromName referring to a RangeAttributeSymbol. That form was the reason the parameter was optional, and it is a range like any other.

Unit Tests

  • test_ScalarConstraint_RangeIsMandatory and test_ConstrainedScalarSubtypeSymbol_withoutConstraint now assert that omitting the constraint raises a TypeError; previously both asserted that Constraint is None.
  • test_ScalarConstraint_WithRangeAttribute covers the range-attribute form.

465 tests pass (was 464).

Notes

range <> — as in type matrix is array (natural range <>) of bit; — is an index_subtype_definition, not a subtype indication. ArrayType models it as a dimension (List[Range]), so it never reaches this class.

This is a breaking change, so the next release is a minor one: v0.39.0 rather than v0.38.1. #168 has to be retitled and its version bump adjusted once this is merged.

pyGHDL.dom needs a companion change. GetScalarConstrainedSubtypeFromNode passes None when GHDL reports the range constraint as an Attribute_Name, with a # TODO: Get actual range from AttributeName node?. Nothing raises, so this is not a crash — but Constraint would hold None in violation of its type. _Translate.py already builds exactly the right object one function further down (RangeFromName(node, RangeAttributeSymbol(node, GetName(node)))), so the fix is a few lines. Filed in the findings notebook.

VHDL's `range_constraint` rule is `**range** range`, so a subtype indication is
either a bare type mark - modelled as `SimpleSubtypeSymbol` - or a type mark
followed by the `range` keyword *and* a range. A form like `integer range`
doesn't exist, which leaves `ConstrainedScalarSubtypeSymbol` without a state
where its constraint is absent.

The `constraint` parameter therefore loses its `None` default and is typed
`Range` instead of `Nullable[Range]`, in `ScalarConstraint` as well as in
`ConstrainedScalarSubtypeSymbol`. The `Constraint` property is non-optional
accordingly.

A range constraint written as a range attribute (`natural range vector'range`)
is a range too: `RangeFromName` referring to a `RangeAttributeSymbol`. It is now
shown as a second example on the class and covered by a testcase, because that
form was the reason the parameter was optional.

Co-Authored-By: Patrick Lehmann <Paebbels@gmail.com>
@codacy-production

codacy-production Bot commented Aug 4, 2026

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 2 critical · 4 minor

Alerts:
⚠ 6 issues (≤ 0 issues of at least minor severity)

Results:
6 new issues

Category Results
Documentation 4 minor
ErrorProne 2 critical

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

🟢 Coverage 100.00% diff coverage · +0.00% coverage variation

Metric Results
Coverage variation +0.00% coverage variation
Diff coverage 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (6e8fca2) 5219 4641 88.93%
Head commit (d7c8157) 5219 (+0) 4641 (+0) 88.93% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#172) 3 3 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.14%. Comparing base (6e8fca2) to head (56d3299).
⚠️ Report is 3 commits behind head on dev.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #172   +/-   ##
=======================================
  Coverage   87.14%   87.14%           
=======================================
  Files          24       24           
  Lines        5219     5219           
  Branches      446      446           
=======================================
  Hits         4548     4548           
  Misses        578      578           
  Partials       93       93           
Flag Coverage Δ
unittests 87.14% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@Paebbels

Paebbels commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

/cc @domWalters

@Paebbels

Paebbels commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

CI: 53 jobs succeeded, 9 skipped, 1 failed — run 30947957072. All unit-test jobs pass on every platform and Python version.

The one failure is PublishToGitHubPages / 📖 Merge multiple contents for publishing, and it is not related to this change:

Branch "claude/mandatory-scalar-constraint" is not allowed to deploy to github-pages due to environment protection rules.

The github-pages environment only accepts deployments from protected branches, so the job is rejected after one second, before any step runs. The same job failed identically on claude/dependency-updates (run 30947167380), which is now merged as #171. Filed as a finding.

Comment thread pyVHDLModel/Symbol.py Outdated
Comment thread pyVHDLModel/Symbol.py Outdated
Co-authored-by: Patrick Lehmann <Paebbels@gmail.com>
@Paebbels
Paebbels merged commit b032248 into dev Aug 4, 2026
50 of 51 checks passed
@Paebbels
Paebbels deleted the claude/mandatory-scalar-constraint branch August 4, 2026 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants