Avoid inserting blank lines in ErrorProneTestHelperSourceFormat auto-fix

[msridhar]

Thanks for the great package of checks! I tried to apply ErrorProneTestHelperSourceFormat and its autofixes to our tests in NullAway, and saw that blank lines get inserted as "". E.g.:

msridhar/NullAway@7c8ac63#diff-409faf90d91de636986e6f96aabde20a120b5442cbc3a32af4db4ca837915fceR21

Would it be possible for the check to not require these blank lines, and for the auto-fix not to insert them? Alternately, could you describe your workflow for writing tests such that the developer does not need to manually write these lines? Do you have a good way to write these tests without manually writing and indenting source lines enclosed in quotes? Thanks so much!

[Stephan202]

Thanks for the feedback @msridhar!

Would it be possible for the check to not require these blank lines, and for the auto-fix not to insert them?

I suppose we could introduce a flag that post-processes the check's suggestions by omitting empty strings. The resultant formatting would then not longer be fully GJF compliant, though.

Alternately, could you describe your workflow for writing tests such that the developer does not need to manually write these lines? Do you have a good way to write these tests without manually writing and indenting source lines enclosed in quotes?

Yeah, indeed we've been relying on auto-fixes instead. For this we use apply-error-prone-suggestions.sh, which heavily relies on specific profiles in our pom.xml, as well as our Error Prone fork.

Off the top of my head the modification to the fork aren't relevant if one wishes to apply the suggestions of a single BugChecker. So for NullAway it would likely suffice to introduce a script/Gradle goal that runs Error Prone with -XepPatchChecks:ErrorProneTestHelperSourceFormat -XepPatchLocation:IN_PLACE. Unfortunately my Gradle skills are effectively nonexistent, so I can't provide more guidance than that.

Anyway, while I'd have to take look closer for the details, I think we'd not be opposed to an optional (disabled-by-default) flag that omits empty lines. Let me know if you'd like to go that route for NullAway; then I'll see what we can do.

NB: while we plan to support JDK 11 for the foreseeable future, ideally we introduce conditional text block support to this check, as that works quite well with IDEA's language injection feature, enabling one to write Java-inside-Java. We have a WIP PR for that: #198.

[msridhar]

Would it be possible for the check to not require these blank lines, and for the auto-fix not to insert them?

I suppose we could introduce a flag that post-processes the check's suggestions by omitting empty strings. The resultant formatting would then not longer be fully GJF compliant, though.

True. My issue is that I think the extra empty strings make the test code less readable, in addition to making them harder to write by hand.

Yeah, indeed we've been relying on auto-fixes instead. For this we use apply-error-prone-suggestions.sh, which heavily relies on specific profiles in our pom.xml, as well as our Error Prone fork.

Cool! Yeah I think we could rig up a similar auto-fix config for our Gradle build. I think we would need something like this, in fact, along with good instructions, as fixing these errors by hand would be pretty annoying (particularly for new contributors). Depending on the overhead imposed we may even want to run in a config where we auto-fix these issues periodically rather than enabling the check by default.

Anyway, while I'd have to take look closer for the details, I think we'd not be opposed to an optional (disabled-by-default) flag that omits empty lines. Let me know if you'd like to go that route for NullAway; then I'll see what we can do.

IMO it would be good to support this feature, just in terms of test code readability. I think we would likely want the flag for NullAway usage (and it's fine for it to be off by default). But, as you can see above, I haven't quite decided what exactly our workflow would be for NullAway and how exactly we would adopt this check. So it's completely fine if adding such a flag is low priority.

NB: while we plan to support JDK 11 for the foreseeable future, ideally we introduce conditional text block support to this check, as that works quite well with IDEA's language injection feature, enabling one to write Java-inside-Java. We have a WIP PR for that: #198.

That would be really cool! Unfortunately we will also be supporting JDK 11 for the foreseeable future so wouldn't be able to take advantage anytime soon.

[Stephan202]

Ack! Then I'll look into adding this feature in the near future. 👍

[rickie]

Hey @msridhar, on your branch msridhar/NullAway@7c8ac63#diff-d8162adf8ce392acc05a12713ac761cde51b50b4250a3d408cb04e66ee8ec85aR78 I see some checks are disabled. I can imagine most of these checks are quite opinionated and therefore may be less useful.

However, I'm curious about the reasons for disabling the checks IdentityConversion and CanonicalAnnotationSyntax 🙂. Maybe there are some things that make them less usable that we don't know of. So have you come across edge cases where these checks turned out not to be useful?

[msridhar]

Hey @msridhar, on your branch msridhar/NullAway@7c8ac63#diff-d8162adf8ce392acc05a12713ac761cde51b50b4250a3d408cb04e66ee8ec85aR78 I see some checks are disabled. I can imagine most of these checks are quite opinionated and therefore may be less useful.

However, I'm curious about the reasons for disabling the checks IdentityConversion and CanonicalAnnotationSyntax 🙂. Maybe there are some things that make them less usable that we don't know of. So have you come across edge cases where these checks turned out not to be useful?

@rickie my primary purpose on that branch was to test the ErrorProneTestHelperSourceFormat check so I was just turning off everything else that was causing warnings on our code 🙂 I did not study any of the other checks carefully, yet. If and when we adopt these checks I would at least add a comment as to the reason for turning off a check. Also it's likely we would adopt the checks gradually and turn them on one by one to ease code review.

[rickie]

That makes sense. Good to know, thanks 😄!