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

Did we break code flows in v2? #194

Closed
michaelcfanning opened this issue Jun 19, 2018 · 5 comments
Closed

Did we break code flows in v2? #194

michaelcfanning opened this issue Jun 19, 2018 · 5 comments
Labels

Comments

@michaelcfanning
Copy link
Contributor

We should be consulting with GrammaTech on this. We specifically removed a code flow 'kind' and a 'target' property. These two v1 values were useful to GrammaTech previously in order to provide icons and special highlighting/other viewer functionality.

We discussed this yet? The issue is that we have an open-ended 'Kind' string which is intended to be user-facing. Some 'kinds' may arguably be inferable today (such as a call being implied by a change in nesting level) but not necessarily.

I agree with this general concern. I didn't like the proliferation of code flow 'kinds' previously and all the semantics we had to document around matching pairs of them, specifying target contents. But I think it's reasonable to enable viewers to provide standard icons/rendering for certain standard things (like a branch).

@michaelcfanning michaelcfanning added the 2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. label Jun 19, 2018
@michaelcfanning
Copy link
Contributor Author

@lgolding this issue is the most urgent in my mind for CSD2. code/thread flows is the most critical part of the format for sophisticated checkers. this scenario needs to be handled very well, otherwise, the analysis that's leading the industry will not derive value from the format.

@ghost
Copy link

ghost commented Aug 27, 2018

@michaelcfanning Is there anything left to do here? We have validated that we can display v2 codeFlows in the viewer, and AFAIK the only issue that's been raised is that we need to restore threadFlowLocation.kind (in some manner -- we're still discussing that). That is covered in #202, "Restore threadFlowLocation.kind".

Can we close this? Did Grammatech raise any other issues (I know the "kind" issue didn't come from them.)

@ghost ghost added the discussion-ongoing label Aug 27, 2018
@ghost ghost self-assigned this Aug 27, 2018
@michaelcfanning
Copy link
Contributor Author

  1. remove any requirement to match kind pairs
  2. make kind an array of unique elements
  3. add sanitizer, source and sink as kinds

make it explicit that nesting level is exclusively used for that concept and doesn't depend on any implied semantics of 'kind'.

@michaelcfanning
Copy link
Contributor Author

@lgolding, we'll close this and #202 once you have a draft that covers all three points documented above

@michaelcfanning
Copy link
Contributor Author

@lgolding per a point you made previously, closing this. all open work is tracked in #202

ghost pushed a commit that referenced this issue Mar 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant