Skip to content

Dataflow: Add documentation for language maintainers.#3850

Merged
semmle-qlci merged 3 commits into
github:masterfrom
aschackmull:dataflow/doc
Jul 2, 2020
Merged

Dataflow: Add documentation for language maintainers.#3850
semmle-qlci merged 3 commits into
github:masterfrom
aschackmull:dataflow/doc

Conversation

@aschackmull

Copy link
Copy Markdown
Contributor

Consensus seem to have emerged to put this document in docs/ql-libraries/dataflow.

@yoff

yoff commented Jun 30, 2020

Copy link
Copy Markdown
Contributor

Some "limitations" that should perhaps be documented: Do not add abstract predicates to Node, the library extends this type and will fail to implement them. Also, it seems that making Node an abstract class creates a non-monotonic recursion.

@hvitved hvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Excellent to have this @aschackmull 🎉

Comment thread docs/ql-libraries/dataflow/dataflow.md
Comment thread docs/ql-libraries/dataflow/dataflow.md Outdated
Comment thread docs/ql-libraries/dataflow/dataflow.md Outdated
Comment thread docs/ql-libraries/dataflow/dataflow.md
Comment thread docs/ql-libraries/dataflow/dataflow.md Outdated
Comment thread docs/ql-libraries/dataflow/dataflow.md Outdated
Comment thread docs/ql-libraries/dataflow/dataflow.md Outdated
Comment thread docs/ql-libraries/dataflow/dataflow.md
Comment thread docs/ql-libraries/dataflow/dataflow.md Outdated
@hvitved

hvitved commented Jun 30, 2020

Copy link
Copy Markdown
Contributor

Some "limitations" that should perhaps be documented: Do not add abstract predicates to Node, the library extends this type and will fail to implement them. Also, it seems that making Node an abstract class creates a non-monotonic recursion.

I can recommend having an internal abstract NodeImpl class for defining predicates by dispatch: https://github.com/github/codeql/blob/master/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll#L19. This also allows us to help the optimizer by telling it that e.g. getEnclosingCallable is functional, if that is not obvious from the implementation.

@jbj jbj requested a review from MathiasVP June 30, 2020 14:09
Comment thread docs/ql-libraries/dataflow/dataflow.md Outdated
Comment thread docs/ql-libraries/dataflow/dataflow.md Outdated
@intrigus-lgtm

Copy link
Copy Markdown
Contributor

I'm curious, what is an IPA type?
Googling did not give me an answer unfortunately.

@MathiasVP

Copy link
Copy Markdown
Contributor

I'm curious, what is an IPA type?

It's jargon for an algebraic datatype: https://help.semmle.com/QL/ql-handbook/types.html#algebraic-datatypes.

@aschackmull

Copy link
Copy Markdown
Contributor Author

Some "limitations" that should perhaps be documented: Do not add abstract predicates to Node, the library extends this type and will fail to implement them. Also, it seems that making Node an abstract class creates a non-monotonic recursion.

I'm not sure that this is an important detail about the library itself, rather given that Node is exposed to the user of the library, it is generally a bad idea to make it abstract or give it abstract predicates as users are likely to e.g. define their sources and sinks as subset of Node by subclassing. So this is just the general rule-of-thumb for QL libraries that public classes never should be abstract unless it is their main use-case to be extended.

@aschackmull

Copy link
Copy Markdown
Contributor Author

I think all current comments should now be addressed.

@hvitved hvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small typo

Comment thread docs/ql-libraries/dataflow/dataflow.md Outdated
Co-authored-by: Tom Hvitved <hvitved@github.com>
@semmle-qlci semmle-qlci merged commit 0bf1f75 into github:master Jul 2, 2020
@aschackmull aschackmull deleted the dataflow/doc branch July 2, 2020 08:04
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.

6 participants