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.
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
Update documentation on data flow in Go (and some small fixes for java) #18511
base: main
Are you sure you want to change the base?
Update documentation on data flow in Go (and some small fixes for java) #18511
Changes from 5 commits
cab9c64
4f2d7ad
9785aac
037ce3d
75424f3
26b8758
549baba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
A potential point for confusion with this example is that it may not be clear to readers whether
temp := x
requires taint flow analysis or justy := ...
.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.
I agree it's an odd example, with the capacity for confusion. I've changed it to just (each language's version of)
y := "Hello " + x
in all the language guides where it appears.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.
Although I'd be surprised if we have a language where (the equivalent of)
temp := x
requires taint-flow analysis, I'd be careful with just changing examples for other languages.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.
I've advertised the change more widely among the language teams.
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.
The wording of this could be improved with something like this:
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.
I slightly prefer the original. It has more narrative flow ("start with a simple query, then slowly add restrictions to get something more interesting"). Can you explain what you don't like about it, and how your suggestion is an improvement?
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.
The narrative is good, and I wouldn't change that. The intention with my suggestion was to keep it as well. However, the flow/grammar/style of the first sentence that's currently here isn't great:
Then ...
feels disconnected here as an opening to the paragraph. It implies that we are continuing some line of work, even though the code example accomplished everything the previous paragraph set out to do. This is why, in my suggestion, the sentence begins by setting out what the next goal is.localFlow
or (the) source discovered by evaluating the query? If a reader assumes the latter case, "the" implies that this query only ever finds one source.Also, while going through this again, does
Parameter
give us parameters or variable accesses to parameters?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.
Parameter
extendsDeclaredVariable
, so I'm pretty sure it's parameters rather than variable accesses to parameters.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.
"This" is potentially ambiguous as it could refer to the query above the text:
I also think that "hard-coded" is a bit ambiguous. One person might understand this to mean "a constant that's provided directly as argument". Your interpretation here is: "a constant that is defined locally in the scope of the function somewhere". Another person might say "a constant that's defined anywhere". We should clarify what the query is intended to find.
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.
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.
Yes, a colon would help, but resolving the ambiguity at the start of the sentence would still be good as well since it then doesn't require the reader to read/scan the entire sentence to know what it's about.
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.
I've tried to do this for multiple languages.
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.
Perhaps there could be a short sentence explaining what
StringOps::Formatting::Range
is or a link to other documentation?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.
Similar comment about "hard-coded" as above.
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.
The passive voice makes this ambiguous. I would write "We can use global data flow by [..]", but if the agreed on style dictates that the passive must be used, then something like "A query can use the global data flow library by [..]"
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.
How about this:
I note that java has this:
You use the global data flow library by implementing the signature ``DataFlow::ConfigSig`` and applying the module ``DataFlow::Global<ConfigSig>``:
. I don't like it, though it is in the active voice.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.
I also don't like using the second person ("You") for this. Third-person, active voice is best in my opinion. Your suggestion with the imperative voice is OK.
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.
Hmm, didn't your suggestion use "we", which is first person, rather than third person?
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.
Sorry, I meant to write "first person plural".
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.
I've adopted your suggestion for all language guides where this sentence appears.
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.
Is there a better description than "restricts"? All the predicates "restrict" data flow in some way.
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.
I've changed this to
optional, defines where data flow is blocked
in all the language guides where it appears.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.
Should we note / recommend
ActiveThreatModelSource
?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.
I think that's more advanced. This guide gives a simple approach that works. (Also, if we want to do that, we should do it separately for all the languages.)
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.
Same concern about "hard-coded".
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.
"Data flows" sounds odd to me. How about "data flow paths" instead?
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.
I've done this for all the language guides where it appears.
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.
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.
I deliberately didn't do this because I think it's a bit harder to understand. I think this guide should just be a simple approach that is easy to understand and which works.
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.
You could mention it as a note underneath the example. "For brevity, we could also shorten
...
to...
".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.
To my mind, the aim of this guide is not to give the best way to write things, but to help the reader use data flow and to be as clear as possible. I think that introducing a new notation to do the same thing does not help with that, especially when it isn't very easy to see at a glance what it is doing.