Skip to content
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

propagate endLine/endColumn #167

Merged
merged 5 commits into from
Apr 9, 2024
Merged

propagate endLine/endColumn #167

merged 5 commits into from
Apr 9, 2024

Conversation

kdudka
Copy link
Member

@kdudka kdudka commented Mar 29, 2024

Initially csdiff supported only startLine/startColumn. Some scanners provide also endLine/endColumn to denote a region of certain size.

The extra information is stored efficiently in the internal data structure and in csdiff's native JSON format by tracking the diff between startLine/endLine and startColumn/endColumn, respectively, and only when the diff is a non-zero (positive) number.

Resolves: #136

@kdudka kdudka requested review from jamacku and lzaoral March 29, 2024 12:36
@kdudka kdudka self-assigned this Mar 29, 2024
kdudka added a commit to kdudka/csdiff that referenced this pull request Mar 29, 2024
kdudka added a commit to kdudka/csdiff that referenced this pull request Apr 1, 2024
kdudka added a commit to kdudka/csdiff that referenced this pull request Apr 3, 2024
Copy link
Member

@jamacku jamacku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this RFE. I have tried to test it, and unfortunately, the endLine/endColumn properties aren't filled for the following shell script.

[root@vm-10-0-187-125 ~]# cat main.sh 
# shell.sh
echo $1                           # Unquoted variables
find . -name *.ogg                # Unquoted find/grep patterns
rm "~/my file.txt"                # Quoted tilde expansion
v='--verbose="true"'; cmd $v      # Literal quotes in variables
for f in "*.ogg"                  # Incorrectly quoted 'for' loops
touch $@                          # Unquoted $@

[root@vm-10-0-187-125 ~]# shellcheck --format=json1 main.sh | csgrep --mode=sarif  | sarif-fmt
error: Couldn't parse this for loop. Fix to allow more checks.
  ┌─ main.sh:6:1
  │
6 │ for f in "*.ogg"                  # Incorrectly quoted 'for' loops
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: Expected 'do'.
  ┌─ main.sh:7:1
  │
7 │ touch $@                          # Unquoted $@
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: Expected 'do'. Fix any mentioned problems and try again.
  ┌─ main.sh:7:1
  │
7 │ touch $@                          # Unquoted $@
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: 0 errors emitted

[root@vm-10-0-187-125 ~]# shellcheck --format=json main.sh | shellcheck-sarif  | sarif-fmt
error: Couldn't parse this for loop. Fix to allow more checks.
  ┌─ main.sh:6:1
  │
6 │ for f in "*.ogg"                  # Incorrectly quoted 'for' loops
  │ ^
  │
  = SC1073
  = For more information: https://www.shellcheck.net/wiki/SC1073

error: Expected 'do'.
  ┌─ main.sh:7:1
  │
7 │ touch $@                          # Unquoted $@
  │ ^
  │
  = SC1058
  = For more information: https://www.shellcheck.net/wiki/SC1058

error: Expected 'do'. Fix any mentioned problems and try again.
  ┌─ main.sh:7:1
  │
7 │ touch $@                          # Unquoted $@
  │ ^
  │
  = SC1072
  = For more information: https://www.shellcheck.net/wiki/SC1072

error: 0 errors emitted

But when I commented last two lines, so ShellCheck could parse the whole file, the properties are set correctly by csgrep

[root@vm-10-0-187-125 ~]# cat main.sh 
# shell.sh
echo $1                           # Unquoted variables
find . -name *.ogg                # Unquoted find/grep patterns
rm "~/my file.txt"                # Quoted tilde expansion
v='--verbose="true"'; cmd $v      # Literal quotes in variables
#for f in "*.ogg"                  # Incorrectly quoted 'for' loops
#touch $@                          # Unquoted $@

shellcheck --format=json1 main.sh | csgrep --mode=sarif  | sarif-fmt
error: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
  ┌─ main.sh:1:1
  │
1 │ # shell.sh
  │ ^^^^^^^^^^

warning: Double quote to prevent globbing and word splitting.
  ┌─ main.sh:2:6
  │
2 │ echo $1                           # Unquoted variables
  │      ^^

warning: Quote the parameter to -name so the shell won't interpret it.
  ┌─ main.sh:3:14
  │
3 │ find . -name *.ogg                # Unquoted find/grep patterns
  │              ^^^^^

warning: Use ./*glob* or -- *glob* so names with dashes won't become options.
  ┌─ main.sh:3:14
  │
3 │ find . -name *.ogg                # Unquoted find/grep patterns
  │              ^

warning: Tilde does not expand in quotes. Use $HOME.
  ┌─ main.sh:4:5
  │
4 │ rm "~/my file.txt"                # Quoted tilde expansion
  │     ^^^^^^^^^^^^^

warning: Quotes/backslashes will be treated literally. Use an array.
  ┌─ main.sh:5:3
  │
5 │ v='--verbose="true"'; cmd $v      # Literal quotes in variables
  │   ^^^^^^^^^^^^^^^^^^

warning: Quotes/backslashes in this variable will not be respected.
  ┌─ main.sh:5:27
  │
5 │ v='--verbose="true"'; cmd $v      # Literal quotes in variables
  │                           ^^

warning: 6 warnings emitted
error: 6 errors emitted

src/lib/defect.hh Outdated Show resolved Hide resolved
src/lib/defect.hh Outdated Show resolved Hide resolved
src/lib/parser-json-gcc.cc Show resolved Hide resolved
src/lib/parser-json-gcc.cc Outdated Show resolved Hide resolved
src/lib/parser-json-gcc.cc Outdated Show resolved Hide resolved
src/lib/parser-json-simple.cc Outdated Show resolved Hide resolved
tests/csgrep/0094-gcc-json-stdout.txt Show resolved Hide resolved
tests/csgrep/0094-gcc-json-stdout.txt Show resolved Hide resolved
kdudka added a commit to kdudka/csdiff that referenced this pull request Apr 3, 2024
... paired with each startLine/startColumn to be compatible
with `shellcheck-sarif` and `sarif-fmt`.

Resolves: csutils#136
Closes: csutils#167
@kdudka
Copy link
Member Author

kdudka commented Apr 3, 2024

@jamacku Thank you for testing it! The problem you pointed out should be fixed in b67c44f.

Copy link
Member

@jamacku jamacku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great, Thank you

[root@vm-10-0-187-125 ~]# shellcheck --format=json1 main.sh | csgrep --mode=sarif  | sarif-fmt
error: Couldn't parse this for loop. Fix to allow more checks.
  ┌─ main.sh:6:1
  │
6 │ for f in "*.ogg"                  # Incorrectly quoted 'for' loops
  │ ^

error: Expected 'do'.
  ┌─ main.sh:7:1
  │
7 │ touch $@                          # Unquoted $@
  │ ^

error: Expected 'do'. Fix any mentioned problems and try again.
  ┌─ main.sh:7:1
  │
7 │ touch $@                          # Unquoted $@
  │ ^

error: 0 errors emitted
[root@vm-10-0-187-125 ~]# shellcheck --format=json1 main.sh | csgrep --mode=sarif  | sarif-fmt
error: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
  ┌─ main.sh:1:1
  │
1 │ # shell.sh
  │ ^

warning: Double quote to prevent globbing and word splitting.
  ┌─ main.sh:2:6
  │
2 │ echo $1                           # Unquoted variables
  │      ^^

warning: Quote the parameter to -name so the shell won't interpret it.
  ┌─ main.sh:3:14
  │
3 │ find . -name *.ogg                # Unquoted find/grep patterns
  │              ^^^^^

warning: Use ./*glob* or -- *glob* so names with dashes won't become options.
  ┌─ main.sh:3:14
  │
3 │ find . -name *.ogg                # Unquoted find/grep patterns
  │              ^

warning: Tilde does not expand in quotes. Use $HOME.
  ┌─ main.sh:4:5
  │
4 │ rm "~/my file.txt"                # Quoted tilde expansion
  │     ^^^^^^^^^^^^^

warning: Quotes/backslashes will be treated literally. Use an array.
  ┌─ main.sh:5:3
  │
5 │ v='--verbose="true"'; cmd $v      # Literal quotes in variables
  │   ^^^^^^^^^^^^^^^^^^

warning: Quotes/backslashes in this variable will not be respected.
  ┌─ main.sh:5:27
  │
5 │ v='--verbose="true"'; cmd $v      # Literal quotes in variables
  │                           ^^

warning: 6 warnings emitted
error: 6 errors emitted

@kdudka kdudka requested review from jamacku and lzaoral April 3, 2024 11:52
@kdudka
Copy link
Member Author

kdudka commented Apr 3, 2024

@jamacku Perfect. Thank you for testing it!

kdudka added a commit to kdudka/csdiff that referenced this pull request Apr 4, 2024
kdudka added a commit to kdudka/csdiff that referenced this pull request Apr 4, 2024
... paired with each startLine/startColumn to be compatible
with `shellcheck-sarif` and `sarif-fmt`.

Resolves: csutils#136
Closes: csutils#167
@lzaoral
Copy link
Member

lzaoral commented Apr 9, 2024

@jamacku This is probably unrelated to this PR but the first output from sarif-fmt in #167 (review) is weird. There three errors but the summary shows:

error: 0 errors emitted

@jamacku
Copy link
Member

jamacku commented Apr 9, 2024

@jamacku This is probably unrelated to this PR but the first output from sarif-fmt in #167 (review) is weird. There three errors but the summary shows:

error: 0 errors emitted

TBH, I'm not sure what those values should represent. It is possible that values might be a bit misleading. For example the second report shows number of errors 6 but there is 7 reports.

Copy link
Member

@lzaoral lzaoral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The comments above are either minor remarks or questions.

Copy link
Member

@jamacku jamacku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, works with sarif-fmt

If no line number is available, skip writing of the `region` object
in order not to break validation of the resulting SARIF format.

Related: csutils#167
Initially csdiff supported only startLine/startColumn.  Some scanners
provide also endLine/endColumn to denote a region of certain size.

The extra information is stored efficiently in the internal data
structure and in csdiff's native JSON format by tracking the diff
between startLine/endLine and startColumn/endColumn, respectively,
and only when the diff is a non-zero (positive) number.

Related: csutils#136
Otherwise fallback to `caret`, which was the only field used so far.

Related: csutils#136
... paired with each startLine/startColumn to be compatible
with `shellcheck-sarif` and `sarif-fmt`.

Resolves: csutils#136
Closes: csutils#167
@kdudka
Copy link
Member Author

kdudka commented Apr 9, 2024

Thank you both for review!

@kdudka kdudka merged commit 6477631 into csutils:main Apr 9, 2024
33 checks passed
@kdudka kdudka deleted the sarif-region branch April 9, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please include region data in SARIF if available 🚦
3 participants