-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Fix/scl language support #1263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix/scl language support #1263
Changes from all commits
c2f11bf
b17766b
813637a
bbd2589
cb08652
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stale site docs & CLA. @WorkRCS |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // Copyright 2026 alibaba/open-code-review Contributors | ||
|
|
||
| package allowedext | ||
|
|
||
| import "testing" | ||
|
|
||
| func TestSCLIsAllowedCaseInsensitive(t *testing.T) { | ||
| for _, ext := range []string{".scl", ".SCL"} { | ||
| t.Run(ext, func(t *testing.T) { | ||
| if !IsAllowedExt(ext) { | ||
| t.Fatalf("IsAllowedExt(%q) = false, want true", ext) | ||
| } | ||
| }) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,5 +109,6 @@ | |
| ".vhdl", | ||
| ".sol", | ||
| ".vy", | ||
| ".rego" | ||
| ".rego", | ||
| ".scl" | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| #### Siemens SCL Review Principles | ||
| > Focus on defects that can change PLC behavior, safety, determinism, or runtime reliability. SCL (Structured Control Language) is a Pascal-based textual PLC language aligned with IEC 61131-3 Structured Text. Do not flag ordinary Pascal-like syntax or vendor-specific conventions unless the changed code demonstrates a concrete correctness problem. | ||
|
|
||
| #### Control Flow and State | ||
| - Conditions that accidentally omit a required branch, especially when the omitted state leaves an output, command, or state variable holding a stale value | ||
| - `CASE` statements that do not cover an expected value when the uncovered state can produce unsafe or incorrect machine behavior | ||
| - State-machine transitions that make a state unreachable, create an unintended transition loop, or leave the machine with no valid successor | ||
| - Repeated writes to the same control variable in one scan where later assignments can silently override an earlier decision | ||
|
|
||
| #### Loops and Scan-Time Safety | ||
| - Loops whose termination depends on mutable PLC state without a clear progress condition | ||
| - Unbounded or unexpectedly large loops in cyclic program execution when they can exceed the expected scan-time budget or starve time-critical logic | ||
| - Busy-wait loops used where a state transition, timer, or other PLC-supported sequencing mechanism is needed | ||
|
|
||
| #### Data Types and Numeric Correctness | ||
| - Implicit conversions or narrowing assignments that can truncate a value, change signedness, or lose precision in a way that affects control logic | ||
| - Arithmetic whose range can exceed the target data type, especially counters, timers, indexes, and accumulated values | ||
| - Array indexing that can reach outside the declared bounds because the index is derived from runtime input or an unchecked calculation | ||
| - Comparisons between values with incompatible or unintended types where the result can differ from the programmer's apparent intent | ||
|
|
||
| #### I/O and Persistent Data | ||
| - Writing outputs or persistent state before required validity, mode, or interlock checks have completed | ||
| - Reading an input once and then using a stale snapshot across logic that can change the relevant machine state within the same scan | ||
| - Resetting, overwriting, or reinitializing retained/process data unexpectedly, causing loss of state across cycles or restarts | ||
|
|
||
| #### Error Handling and External Blocks | ||
| - Ignoring a status/error result from a called block or communication operation when the caller continues as though the operation succeeded | ||
| - Using data produced by a block, timer, counter, or communication interface before establishing that the result is valid for the current state | ||
| - Error paths that leave an actuator command, mode bit, or state variable in an unsafe or contradictory condition | ||
|
|
||
| #### Maintainability with Behavioral Impact | ||
| - Duplicated control conditions that can diverge after a change and cause two parts of the sequence to make conflicting decisions | ||
| - Magic constants used for operational limits where the value is part of a safety, timing, or process invariant and can be changed independently of the corresponding logic | ||
| - Dead branches or unreachable code when their presence can hide a missing transition or an incomplete safety condition |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is definitely a exclude pattern because SCL follows the parallel structure, like this: |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // Copyright 2026 alibaba/open-code-review Contributors | ||
|
|
||
| package rules | ||
|
|
||
| import ( | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestResolve_SCLRule(t *testing.T) { | ||
| rule, err := LoadDefault() | ||
| if err != nil { | ||
| t.Fatalf("LoadDefault: %v", err) | ||
| } | ||
|
|
||
| for _, path := range []string{"main.scl", "plc/Blocks/Motor.SCL", "src/control.scl"} { | ||
| t.Run(path, func(t *testing.T) { | ||
| got := rule.Resolve(path) | ||
| if !strings.Contains(got, "Siemens SCL Review Principles") { | ||
| t.Fatalf("Resolve(%q): expected Siemens SCL rule, got %q", path, truncate(got, 120)) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do need a standalone test? This is already covered by tests, you just need to append them to the test index.