📝 Notice about subtleties of inversion semantics of ForStatement#545
📝 Notice about subtleties of inversion semantics of ForStatement#545TooMuchDakka wants to merge 4 commits intomunich-quantum-toolkit:mainfrom
ForStatement#545Conversation
…uring inversion of ForStatement during synthesis of UncallStatement
📝 WalkthroughWalkthroughDocumentation updates across multiple files transition public API references from snake_case to CamelCase naming conventions (e.g., Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/syrec_language_semantics.md`:
- Around line 303-305: The explanatory comment describing the inverted
ForStatement contains a typo: it uses `++b.$i` but elsewhere the
increment-assignment syntax is `++= b.$i`; update the comment to use `++= b.$i`
(and verify the paired decrement `--= a.$i` is correct) so the comment matches
the SyReC syntax and avoids confusion when referring to the loop variable `$i`
and the ForStatement inversion.
… warning section of ForStatement
|
|
||
| - The value of the step size of a ForStatement cannot be defined as or evaluate to 0 to prevent an infinite loop. | ||
|
|
||
| ````{warning} |
There was a problem hiding this comment.
@TooMuchDakka This seems to cause Markdown formatting issues:
| ````{warning} |
(At least it doesn't render correctly here on GitHub.)
There was a problem hiding this comment.
I do not see any markdown formatting issues (see screenshot below) when performing a local build of the docs via nox -s docs.
There was a problem hiding this comment.
yeah, this is fine and typical nesting syntax
…ascalCasing were appropriate
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/library/Parser.rst`:
- Line 6: The docs file name is inconsistent: change Parser.rst to Program.rst
and update any references; specifically rename the file currently named
Parser.rst to Program.rst so it matches the documented class mqt.syrec.Program,
and search for/replace any cross-references or toctree entries that point to
"Parser" to point to "Program" instead (ensure the autoclass line "..
autoclass:: mqt.syrec.Program" remains unchanged).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
docs/library/AnnotatableQuantumComputation.rstdocs/library/ConfigurableOptions.rstdocs/library/InlinedQubitInformation.rstdocs/library/NBitValuesContainer.rstdocs/library/Parser.rstdocs/library/Statistics.rst
…tation and the actual python bindings
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
I am not sure whether this actually points to a bug. Maybe you can clarify.
|
|
||
| - The value of the step size of a ForStatement cannot be defined as or evaluate to 0 to prevent an infinite loop. | ||
|
|
||
| ````{warning} |
There was a problem hiding this comment.
yeah, this is fine and typical nesting syntax
| Uncalling modules containing a _ForStatement_ could lead to unexpected index-out-of-range errors if one is not familiar with the semantics of how a _ForStatement_ or more specifically its value range is inverted, with the inversion semantics being inherited by SyReC from its reversible programming language predecessor Janus {cite:p}`yokoyama2007janus`. | ||
|
|
||
| A loop (_ForStatement_) defined as {math}`for \ e_1 \ to \ e_2 \ step \ s \ do \ s_1, s_2, \dots, s_n \ rof` | ||
| is inverted by inverting not only the sequence of statements in its loop body but also by inverting its value range thus the inversion of the given loop is equal to | ||
| {math}`for \ e_2 \ to \ e_1 \ step \ s \ do \ {s_n}^{-1}, \dots, {s_2}^{-1}, {s_1}^{-1} \ rof`. | ||
|
|
||
| The inversion result of the _ForStatement_ as well as its enclosing module `basicBitwiseIncr` is equal to the module `invrBasicBitwiseIncr` with both modules being shown in the example below: | ||
|
|
||
| ```text | ||
| module basicBitwiseIncr(inout a(4), inout b(4)) | ||
| for $i = 0 to #a do | ||
| ++= a.$i; | ||
| --= b.$i | ||
| rof | ||
|
|
||
| // Inversion of basicBitwiseIncr equal to "uncall basicBitwiseIncr(a, b)" | ||
| module invrBasicBitwiseIncr(inout a(4), inout b(4)) | ||
| for $i = #a to 0 do | ||
| ++= b.$i; | ||
| --= a.$i | ||
| rof | ||
|
|
||
| module main(inout a(4), inout b(4)) | ||
| // An index-out-of-range error would be reported here due to the value range of the ForStatement of the uncalled module | ||
| // being inverted with the loop variable $i being initialized with #a instead of 0 in the inverted ForStatement thus the assignments | ||
| // ++= b.$i and --= a.$i would both result in an index-out-of-range error. | ||
| uncall basicBitwiseIncr(a, b) | ||
| ``` |
There was a problem hiding this comment.
Is this really true? This seems like more of a bug to me.
shouldn't the reversed loop be from #a-1 to -1?
If the regular loop contains only valid indexing operations, then the inverse of that loop should as well.
Markdown syntax doesn't render properly for me. |
It isn't supposed to if you just look at the markdown file itself. This file is rendered by MyST as part of the documentation build. As @TooMuchDakka has shown in his screenshot, this renders just fine in the resulting documentation. |
|
@burgholzer Oh interesting. I didn't know about MyST. |
denialhaag
changed the title
ForStatementForStatement
3 participants
Description
While we do mention that the inversion semantics of the
UncallStatementin SyReC were inherited from its predecessor Janus, some subtleties about said semantics can easily be overlooked especially forForStatementswhich in turn can result in unexpected errors with one example being shown here.An additional warning section regarding this potentially unexpected behaviour during the inversion of a
ForStatementwill be added with this PR to the documentation of the semantics of theForStatement.Furthermore, with this PR the library documentation is updated by replacing the previously defined snake_cased identifiers of the generated python bindings with their correct PascalCased identifiers (introduced with #523) where appropriate.
Checklist: