SpanMarkPointMissingMaskDetector - Ensure that span flags are checked correctly #14

slack-jallen · 2021-12-15T17:10:16Z

Summary

Ensure that span flags are checked correctly.

❌ Incorrect
spanned.getSpanFlags(span) == Spanned.INCLUSIVE_INCLUSIVE

✅ Correct
spanned.getSpanFlags(span) and Spanned.SPAN_POINT_MARK_MASK == Spanned.INCLUSIVE_INCLUSIVE

❓ Why
Spans flags may have priority bits or other bits set.

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

The following point can be removed after setting up CI (such as Travis) with coverage reports (such as Codecov)

I've written tests to cover the new code and functionality included in this PR.

The following point can be removed after setting up a CLA reporting tool such as cla-assistant.io

I've read, agree to, and signed the Contributor License Agreement (CLA).

…an checks.

ZacSweers

Nice work! Mostly little nits and suggestions

slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerousCheckDetector.kt

slack-lint/src/test/java/slack/lint/text/SpanPointMarkDangerousCheckDetectorTest.kt

slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerousCheckDetector.kt

slack-lint/src/test/java/slack/lint/text/SpanPointMarkDangerousCheckDetectorTest.kt

slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerousCheckDetector.kt

ZacSweers

Whoops sorry didn't mean to approve just yet

…sCheckDetector.kt Co-authored-by: Zac Sweers <pandanomic@gmail.com>

…issue

- make this name match the others - move class out to be top-level in the file - check UastBinaryOperator.EQUALS - functions can all be private - raw string - use the in operator - make this an extension function - rename to resolveQualifiedNameOrNull() - use a reference expression here

- exercises different levels of imports (constant directly, class + reference, fully qualified reference). - visitBinaryExpression - prefix on all of these are redundant - remove @language("kotlin") - indent inconsistency - keep these names simple

…llen/slack-lints into hunvil-and-josh/span-point-mark

No need to call .trimIndent() in issue registration strings; they are already trimmed by indent by lint when displaying to users [LintImplTrimIndent]

ZacSweers · 2022-01-18T17:10:41Z

Let's make these commits finer grained in the future (i.e. only logical groupings, not buckets). Especially with the main merge.

ZacSweers · 2022-01-18T17:11:57Z

slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerousCheckDetector.kt

+        Spans flags can have priority or other bits set. \
+        Ensure that Span flags are checked using \
+        `currentFlag and Spanned.SPAN_POINT_MARK_MASK == desiredFlag` \
+        rather than just `currentFlag == desiredFlag`


ZacSweers · 2022-01-18T17:12:39Z

slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerousCheckDetector.kt

+/**
+ * Checks for SpanPointMarkDangerousCheck. See [ISSUE].
+ */
+class SpanPointMarkDangerousCheckDetector : Detector(), SourceCodeScanner {


I've been thinking about this and I think "dangerous" is too strong of a word here. Let's pick something else, like "incorrect" or "missing mask check", something to that effect that's more descriptive

How do you like SpanMarkPointMissingMaskDetector?

sounds good to me 👍

👍 Fixed in 961528c and 34bb19e

ZacSweers · 2022-01-18T17:12:49Z

slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerousCheckDetector.kt

+/**
+ * Checks for SpanPointMarkDangerousCheck. See [ISSUE].
+ */


nit:

Suggested change

/**

* Checks for SpanPointMarkDangerousCheck. See [ISSUE].

*/

/** Checks for SpanPointMarkDangerousCheck. See [ISSUE]. */

👍 Fixed in affa6e8

ZacSweers · 2022-01-18T17:13:02Z

slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerousCheckDetector.kt

+  companion object {
+    val ISSUE = Issue.create(
+      id = "SpanPointMarkDangerousCheck",
+      briefDescription = "Check Span Flags Using Bitmask",


This should be a sentence not a title

👍 Fixed in f4b4da4

ZacSweers · 2022-01-18T17:13:36Z

slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerousCheckDetector.kt

+  private fun matchesMarkPoint(expression: UExpression): Boolean {
+    return getQualifiedName(expression) in MARK_POINT_FIELDS
+  }


Suggested change

private fun matchesMarkPoint(expression: UExpression): Boolean {

return getQualifiedName(expression) in MARK_POINT_FIELDS

}

private fun matchesMarkPoint(expression: UExpression) = getQualifiedName(expression) in MARK_POINT_FIELDS

👍 Fixed in 1ff3397

ZacSweers · 2022-01-18T17:15:07Z

slack-lint/src/main/java/slack/lint/util/UExpressionExtensions.kt

+import org.jetbrains.uast.UReferenceExpression
+import org.jetbrains.uast.tryResolve
+
+fun UExpression.resolveQualifiedNameOrNull(): String? {


Add this to an existing util file rather than its own dedicated one. Also please write a short doc

👍 Fixed in 130aa73

ZacSweers · 2022-01-18T17:15:27Z

slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerousCheckDetector.kt

+    }
+  }
+
+  private fun getQualifiedName(expression: UExpression): String? {


Should this be an extension?

Do you prefer an extension in util file or private extension in this file?

👍 Fixed in 7804c86

ZacSweers · 2022-01-18T17:16:06Z

slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerousCheckDetector.kt

+    return getQualifiedName(expression) in MARK_POINT_FIELDS
+  }
+
+  private fun matchesMask(expression: UExpression): Boolean {


extension? Also seems like this should be "isBinaryExpressionWithMaskOperand" rather than matches? Descriptiveness is better here

ditto, Do you prefer an extension in util file or private extension in this file?

this isn't reusable elsewhere so keep it here. Same with the other ones

👍 Fixed in a4ed477 and bc78adc

ZacSweers · 2022-01-18T17:18:06Z

slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerousCheckDetector.kt

+    }
+  }
+
+  private fun matchesMarkPoint(expression: UExpression): Boolean {


Extension + maybe rename to isMarkPointFieldName?

ditto, Do you prefer an extension in util file or private extension in this file?

👍 Fixed in 4a7d8ac and 86bb369

slack-jallen · 2022-01-18T20:48:19Z

Especially with the main merge.

What do you mean by especially with the merge?

ZacSweers · 2022-01-18T20:59:05Z

I believe after a merge, github kind of loses track of how to link shas and I get a page like this. Comments here don't appear to show up in the regular review. So, the better isolated they are the better because it's easier to review quickly. Large "address CR" commits are noisy and essentially require a re-review rather than just confirmation.

slack-jallen · 2022-01-19T02:15:33Z

@ZacSweers Thanks for your feedback! I learned a couple things. Note that I usually take a more reserved approach to extension functions, as they make polymorphism down the road more difficult.

ZacSweers · 2022-01-19T19:16:43Z

Note that I usually take a more reserved approach to extension functions, as they make polymorphism down the road more difficult.

Not sure I follow. In bytecode, these are the same and they're just purely functional

fun computeStuff(param: String): String = ...


fun String.computeStuff(): String = ...

ZacSweers · 2022-01-19T19:22:44Z

Nice work!

slack-jallen · 2022-01-19T19:24:08Z

Note that I usually take a more reserved approach to extension functions, as they make polymorphism down the road more difficult.

Not sure I follow. In bytecode, these are the same and they're just purely functional
fun computeStuff(param: String): String = ...


fun String.computeStuff(): String = ...

What I mean is:

open class SomeAwesomeClass {
   fun doStuff()...
   open fun doOtherStuff()...
}

class MyClassThatDoesWhatIWant {
   override fun doOtherStuff()....
}

// ... dependency inject MyClassThatDoesWhatIWant instead of SomeAwesomeClass

vs.

class SomeAwesomeClass {
}

fun SomeAwesomeClass.doStuff()...
fun SomeAwesomeClass.doOtherStuff()...

... Oh no, SomeAwesomeClass doesn't do what I want, but I can't use polymorphism to change it

slack-jallen added 2 commits December 14, 2021 21:59

Add SpanPointMarkDangerousCheckDetector to guard against dangerous sp…

78fdfcd

…an checks.

Use UExpression fields instead of regex

11522b4

slack-jallen marked this pull request as ready for review December 15, 2021 17:14

slack-jallen changed the title ~~Hunvil and josh/span point mark~~ SpanPointMarkDangerousCheckDetector - Ensure that span flags are checked correctly Jan 3, 2022

ZacSweers previously approved these changes Jan 5, 2022

View reviewed changes

ZacSweers self-requested a review January 5, 2022 17:35

ZacSweers reviewed Jan 5, 2022

View reviewed changes

slack-jallen and others added 6 commits January 6, 2022 11:25

Update slack-lint/src/main/java/slack/lint/text/SpanPointMarkDangerou…

ea04e70

…sCheckDetector.kt Co-authored-by: Zac Sweers <pandanomic@gmail.com>

code review comments - use raw string, use this pattern for creating …

dde29a5

…issue

Code review comments

3727705

- exercises different levels of imports (constant directly, class + reference, fully qualified reference). - visitBinaryExpression - prefix on all of these are redundant - remove @language("kotlin") - indent inconsistency - keep these names simple

Merge branch 'hunvil-and-josh/span-point-mark' of github.com:slack-ja…

d5d9de3

…llen/slack-lints into hunvil-and-josh/span-point-mark

remove trimIndent

b37849d

No need to call .trimIndent() in issue registration strings; they are already trimmed by indent by lint when displaying to users [LintImplTrimIndent]

ZacSweers reviewed Jan 18, 2022

View reviewed changes

slack-jallen added 12 commits January 18, 2022 20:31

Code review comment: add this to the existing utils classes

130aa73

Code review comment: rename to SpanMarkPointMissingMaskDetector

961528c

Code review comment: rename to SpanMarkPointMissingMaskDetector

34bb19e

Code review comment: doc

affa6e8

Code review: should be a sentence not a title

f4b4da4

Code review: use function expression

1ff3397

Code review: extension function

7804c86

Code review: matchesMarkPoint extension function

4a7d8ac

Code review: rename matchesMarkPoint to isMarkPointFieldName

86bb369

Code review: matchesMask extension function

a4ed477

Code review: rename matchesMask to isMaskClass

bc78adc

Code review: createUastHandler function expression

dcbfe2a

ZacSweers changed the title ~~SpanPointMarkDangerousCheckDetector - Ensure that span flags are checked correctly~~ SpanMarkPointMissingMaskDetector - Ensure that span flags are checked correctly Jan 19, 2022

ZacSweers approved these changes Jan 19, 2022

View reviewed changes

ZacSweers merged commit 8500848 into slackhq:main Jan 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SpanMarkPointMissingMaskDetector - Ensure that span flags are checked correctly #14

SpanMarkPointMissingMaskDetector - Ensure that span flags are checked correctly #14

slack-jallen commented Dec 15, 2021 •

edited

ZacSweers left a comment

ZacSweers left a comment

ZacSweers commented Jan 18, 2022 •

edited

ZacSweers Jan 18, 2022

ZacSweers Jan 18, 2022

slack-jallen Jan 18, 2022

ZacSweers Jan 18, 2022

slack-jallen Jan 19, 2022

ZacSweers Jan 18, 2022

slack-jallen Jan 19, 2022

ZacSweers Jan 18, 2022

slack-jallen Jan 19, 2022

ZacSweers Jan 18, 2022

slack-jallen Jan 19, 2022 •

edited

ZacSweers Jan 18, 2022

slack-jallen Jan 19, 2022

ZacSweers Jan 18, 2022

slack-jallen Jan 18, 2022 •

edited

slack-jallen Jan 19, 2022

ZacSweers Jan 18, 2022

slack-jallen Jan 18, 2022 •

edited

ZacSweers Jan 18, 2022

slack-jallen Jan 19, 2022

ZacSweers Jan 18, 2022

slack-jallen Jan 18, 2022 •

edited

slack-jallen Jan 19, 2022 •

edited

slack-jallen commented Jan 18, 2022

ZacSweers commented Jan 18, 2022

slack-jallen commented Jan 19, 2022

ZacSweers commented Jan 19, 2022

ZacSweers commented Jan 19, 2022

slack-jallen commented Jan 19, 2022

SpanMarkPointMissingMaskDetector - Ensure that span flags are checked correctly #14

SpanMarkPointMissingMaskDetector - Ensure that span flags are checked correctly #14

Conversation

slack-jallen commented Dec 15, 2021 • edited

Summary

Requirements (place an x in each [ ])

ZacSweers left a comment

Choose a reason for hiding this comment

ZacSweers left a comment

Choose a reason for hiding this comment

ZacSweers commented Jan 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slack-jallen Jan 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slack-jallen Jan 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slack-jallen Jan 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slack-jallen Jan 18, 2022 • edited

Choose a reason for hiding this comment

slack-jallen Jan 19, 2022 • edited

Choose a reason for hiding this comment

slack-jallen commented Jan 18, 2022

ZacSweers commented Jan 18, 2022

slack-jallen commented Jan 19, 2022

ZacSweers commented Jan 19, 2022

ZacSweers commented Jan 19, 2022

slack-jallen commented Jan 19, 2022

slack-jallen commented Dec 15, 2021 •

edited

Requirements (place an `x` in each `[ ]`)

ZacSweers commented Jan 18, 2022 •

edited

slack-jallen Jan 19, 2022 •

edited

slack-jallen Jan 18, 2022 •

edited

slack-jallen Jan 18, 2022 •

edited

slack-jallen Jan 18, 2022 •

edited

slack-jallen Jan 19, 2022 •

edited