A text symbol is a character literal - read it into a variable first - #849
Merged
Merged
Conversation
On some releases a SUCCESSFUL dynamic `ASSIGN` does not reset `sy-subrc` (abap2UI5 #1937), so `IF sy-subrc = 0` after one can read FALSE for an assignment that worked and TRUE for one that did not. `IS ASSIGNED` is the check that holds on all of them. Eleven sites across eight sample apps tested sy-subrc instead - among them the sub-app view handover (`MV_VIEW_DISPLAY`, `VIEW_PARENT`), which is exactly the failure the comment above one of them already describes for a different cause. Two need more than the drop-in: - app_502's node builder assigns inside `DO 6 TIMES`. A failed assign leaves the previous round's binding in place, so `IS ASSIGNED` would read TRUE for the failure and build onto the wrong node - `UNASSIGN` first. - app_212 dereferenced `mt_table->*` and `ms_table_row->*` and then USED both without ever checking: reading `<tab>[ index ]` or a component of an unassigned field symbol is a short dump, not a catchable exception, so the popup would take the roundtrip down with it. Both are checked now, and the row structure - which does not change per field - is read once before the loop instead of on every iteration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UxxNLTJFLpefuUNnRKqC61
The extended program check (SLIN / ATC) runs in the systems these samples are INSTALLED on and nowhere here - abaplint does not model it. So a sample can pass every gate in this repository and still light up a customer's ATC run, which is worse than a missing sample: it is the code somebody copied because it was published as the way to do the thing. Twenty-one `SELECT` statements read a small demo table with no `WHERE`, which is exactly what they are demonstrating - and none of them said so. The check wants `"#EC CI_NOWHERE` on the statement; the framework's own z2ui5_cl_ui5_srv_draft=>count_entries_total is the precedent, reading the whole draft table on purpose and carrying the pseudo-comment for it. `npm run check:atc` holds it. It decides one finding, the one this repository actually carried, and its scan is line-based on purpose: a SELECT runs from the keyword to the first line ending in a period and carries no string literal that could hide one, so a full statement splitter would buy nothing and cost a reader twenty lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UxxNLTJFLpefuUNnRKqC61
The gate this repository gained for `SELECT` without `WHERE` grows the second finding the same sweep produced. On some releases a SUCCESSFUL dynamic `ASSIGN` does not reset `sy-subrc`, so a test on it reads FALSE for an assignment that worked and TRUE for one that did not (abap2UI5 #1937). Fourteen of those shipped here - among them the sub-app view handover (`MV_VIEW_DISPLAY`, `VIEW_PARENT`), and app_502's node builder, which assigns inside a `DO` where a stale binding builds onto the wrong node. `ASSIGN COMPONENT … OF STRUCTURE` is the negative and is never reported: there `sy-subrc` distinguishes "component not found" and IS the documented check, so reporting it is how a cleanup turns a wrong-branch bug into a silently-taken one. That is also why the scan now reads ABAP STATEMENTS rather than lines - a multi-line `ASSIGN COMPONENT` looks like a plain `ASSIGN` to a line-based scan, which would report exactly the shape that must not be reported. Verified by re-introducing one of each: the gate names both, with the file, the line and the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UxxNLTJFLpefuUNnRKqC61
A Code Inspector SYNTAX_CHECK on a real system reported app 519's view_display: `'Your name'(001) is not type-compatible with formal parameter "V"` - a SYNTAX_ERROR of the whole class, on the sample whose subject IS translatable texts, while abaplint, the transpiler and the unit suite were all green on it. None of them models the compatibility rule for a text-symbol literal: it is a CHARACTER literal, and the view builder's `v` is TYPE string. The three symbols are read into variables and the variables are passed. That is not just the repair, it is the better teaching shape - an app that uses a text twice would write it the same way - and the comment now says why, and says what needs no variable: inside a string template (which on_event( ) does five lines up) an embedded expression is a general expression position. `check:atc` grows the rule as its third. A PARAMETER binding only, decided by whether the binding sits inside an open paren: `lv_x = 'y'(001).` is an assignment and a plain conversion, correct on every release. Verified by putting the symbol back: the gate names the file, the line and the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UxxNLTJFLpefuUNnRKqC61
The gate was only in the local `npm run check` chain, and this repository runs one workflow per gate for a reason its own check-abapdoc header states: a gate that exists only in the chain cannot turn a pull request red. Same shape as check-keywords - plain node, no dependencies, a few seconds - and the header names what each of the three rules found here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UxxNLTJFLpefuUNnRKqC61
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four findings, three of them from real systems, plus the gate that keeps each one out.
'Your name'(001)passed to the view builder'sv(app 519)A Code Inspector
SYNTAX_CHECKreported'Your name'(001) is not type-compatible with formal parameter "V"— aSYNTAX_ERRORof the whole class, on the sample whose subject is translatable texts, while abaplint, the transpiler and the unit suite were all green on it. None of them models the compatibility rule for a text-symbol literal: it is a CHARACTER literal, andvisTYPE string.The three symbols are read into variables and the variables are passed. That is also the better teaching shape — an app that uses a text twice writes it the same way — and the comment now says what needs no variable: inside a string template (which
on_event( )does five lines up) an embedded expression is a general expression position.sy-subrcafter a dynamicASSIGN(14 sites)On some releases a SUCCESSFUL assign does not reset
sy-subrc, so the test reads FALSE for an assignment that worked (abap2UI5 #1937). Among them the sub-app view handover (MV_VIEW_DISPLAY,VIEW_PARENT), and app 502's node builder, which assigns inside aDOwhere a stale binding builds onto the wrong node.Two dereferences used without ever being checked (app 212)
mt_table->*andms_table_row->*were dereferenced and then used: reading<tab>[ index ]or a component of an unassigned field symbol is a short dump, not a catchable exception, so the popup took the roundtrip down with it. Both are checked now, and the row structure — which does not change per field — is read once before the loop instead of on every iteration.21
SELECTs with noWHEREThe extended program check wants
"#EC CI_NOWHEREon the statement; these read a small demo table on purpose, which is exactly what they demonstrate, and none of them said so.z2ui5_cl_ui5_srv_draft=>count_entries_totalis the precedent.The gate
npm run check:atcdecides all three static shapes. It reads ABAP statements, not lines — a multi-lineASSIGN COMPONENTlooks like a plainASSIGNto a line-based scan, and that is the one shape that must never be reported, because theresy-subrcis the documented check. Each rule was verified by putting one finding back.How to test
npm run check— green,check:atcincluded.🤖 Generated with Claude Code
https://claude.ai/code/session_01UxxNLTJFLpefuUNnRKqC61
Generated by Claude Code