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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for lombok.fieldDefaults.default* in lombok.config #571

Merged
merged 5 commits into from Jan 5, 2019

Conversation

@tlinkowski
Copy link
Contributor

commented Dec 27, 2018

This fixes #353 and #456.

This PR consists of four commits:

  1. Extracted configsystem tests to "before" directories + adapted AbstractLombokConfigSystemTestCase accordingly (d6bd9e2, TDD: refactor)

    This changes the layout of "testData/configsystem" directory from:

    configSystem
      ...
      value.extraPrivate
        after
          ValueTest.java
        ValueTest.java
        lombok.config
      ...

    to

    configSystem
      ...
      value.extraPrivate
        before
          ValueTest.java
          lombok.config
        after
          ValueTest.java
      ...

    This change was necessary for this fix because - in the original layout - lombok.config was affecting the files in the "after" directory too. Until now, it wasn't a problem because nearly all lombok.config flags affect only classes that contain certain Lombok annotations. However, lombok.fieldDefaults.default* flags affect every file.

    If you checkout this commit, you can verify that all tests still pass.

  2. Introduced configsystem/FieldDefaultsTests (ede994e, TDD: red)

    Added all the tests for lombok.fieldDefaults.default*. The results of calling the tests at this stage are as follows:
    FieldDefaultsTests initial results

    Explanation:

    • two tests ending in *WithFieldDefaultsTest pass because they test if putting @FieldDefaults(makeFinal = true) annotation overrides defaultFinal in lombok.config
    • DefautFinalFieldWithUnrelatedFieldDefaultsTest pass because putting @FieldDefaults(level = ...) contains an implicit makeFinal = false which overrides defaultFinal in lombok.config

    If you checkout this commit, you can verify that the results of the tests are as above.

  3. Adapted FieldDefaultsModifierProcessor (e6db3ae, TDD: green)

    Please note that I replaced PsiAnnotationUtil.getAnnotationValues(fieldDefaultsAnnotation, "level", String.class) with PsiAnnotationUtil.getStringAnnotationValue(fieldDefaultsAnnotation, "level") (it seemed safe because only one value can be present there, but I'm not 100% sure it is).

    If you checkout this commit, you can verify that all the tests in FieldDefaultsTests now pass.

  4. Refactorings in FieldDefaultsModifierProcessor and AbstractLombokParsingTestCase (f3ac2ef, TDD: refactor)

    This is optional but:

    • I believe refactorings in FieldDefaultsModifierProcessor make it more readable
    • the refactoring in AbstractLombokParsingTestCase changes the parent type in compareModifiers from PsiMethod to PsiElement - without this change, the tests do not display the name of the field for which the modifiers are different

    If you checkout this commit, you can verify that all the tests still pass.


I hope you'll find this PR useful 馃檪 If there were anything that needed improvement, feel free to let me know.

@tlinkowski

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

Sorry, I should've run all the tests after introducing my changes. Apparently, my PR breaks something in FieldDefaultsModifierTest. I'm looking into it.

@tlinkowski

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

OK, for some strange reason, PsiAnnotationUtil.getStringAnnotationValue(fieldDefaultsAnnotation, "level") returned null.

The problem was not in psiAnnotation.findAttributeValue (which always finds the attribute because @FieldDefaults has a default value for level), but in PsiAnnotationUtil.resolveElementValue which couldn't properly resolve PsiReferenceExpression:AccessLevel.NONE.

However, when I changed FieldDefaultsModifierTest to extend AbstractLombokLightCodeInsightTestCase (instead of LightCodeInsightFixtureTestCase), it worked just fine.

@mplushnikov, @alexejk: Are you aware, if null coming from PsiAnnotationUtil.getStringAnnotationValue is something that can be expected in production, or is it only a quirk in testing?

@codecov-io

This comment has been minimized.

Copy link

commented Dec 27, 2018

Codecov Report

Merging #571 into master will increase coverage by 0.11%.
The diff coverage is 95%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #571      +/-   ##
===========================================
+ Coverage     64.38%   64.5%   +0.11%     
- Complexity     1488    1505      +17     
===========================================
  Files           178     178              
  Lines          5298    5310      +12     
  Branches       1182    1185       +3     
===========================================
+ Hits           3411    3425      +14     
+ Misses         1380    1378       -2     
  Partials        507     507
Impacted Files Coverage 螖 Complexity 螖
...essor/modifier/FieldDefaultsModifierProcessor.java 89.79% <95%> (+6.01%) 33 <20> (+16) 猬嗭笍
.../intellij/plugin/lombokconfig/ConfigDiscovery.java 82.71% <0%> (+1.23%) 27% <0%> (+1%) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 3910f11...7d569bd. Read the comment docs.

@mplushnikov

This comment has been minimized.

Copy link
Owner

commented Jan 3, 2019

@tlinkowski Thank you very much for your help! I'll look on your PR on weekend.

@mplushnikov

This comment has been minimized.

Copy link
Owner

commented Jan 5, 2019

Great PR, very well documentated! Thank you very much! Make more of this:)

Are you aware, if null coming from PsiAnnotationUtil.getStringAnnotationValue is something that can be expected in production, or is it only a quirk in testing?

As I know this is mostly problem in the tests only. Using AbstractLombokLightCodeInsightTestCase is absolutly right, because it setups all of lombok classes (and jdk mock) for the test run. Without this setup Intellij can't not find any of classes used i the test and a lot of "resolve" operations fails. Thank you for fixing this in the test too!

Did you tried your build in some big projects? I will merge this now, but I am a little bit afraid of some possible performance degradation, because this code will be running now for every field in every class and searching for lombok configuration information again and again. It could be vissible degradation on big classes with a lot of fields for example. ConfigDiscovery operates on an index and should be fast, but I got already a lot of performance issues for the plugin and comment like this: #516 (comment) are not really reassuring...

@mplushnikov mplushnikov merged commit c2201cb into mplushnikov:master Jan 5, 2019

3 checks passed

codecov/patch 95% of diff hit (target 64.38%)
Details
codecov/project 64.5% (+0.11%) compared to 3910f11
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tlinkowski

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2019

Thank you, Michail, for your kind words 馃槂

As to performance issues: no, I have not tried this build at all. Actually, I have not even considered the consequences with respect to performance - you made an excellent observation!


Update: I have built the plugin (IJ 2018.3.2), and tested it on a fairly large project. When I opened a file with 10+ non-static fields, I think I have experienced some minor slowdown while loading the file (but I can't be sure).


I took a quick look at ConfigDiscovery and ConfigIndexKey, and I started to wonder: would it be technically possible to add an extra layer of indexing per every directory? Let me explain...

As far as I understand, the current index (let's call it index A) stores the contents of lombok.config files as:

  • key: pair(directory containing lombok.config, lombok configuration key)
  • value: lombok configuration value

The index I have in mind (let's call it index B) would use index A to determine the effective values of lombok configuration keys for any given directory (not necessarily containing a lombok.config file) as:

  • key: pair(directory, lombok configuration key)
  • value: effective lombok configuration value

In other words, I would like to move the logic of ConfigDiscovery.discoverProperty/discoverProperties (bubbling) to index B. It would be a kind of "anti-bubbling" index.

But I really don't know if such things are possible within the IntelliJ API so this proposal might be nonsense. And such an approach would pose challenges (e.g. when given lombok.config changed, all entries in the given directory and all its subdirectories, recursively, would have to be updated in index B).

Note that since this index would need to contain all the affected directories in the project, it'd be much bigger than index A (but I can't tell if this could be a problem).

@tlinkowski tlinkowski deleted the tlinkowski:fix-353 branch Jan 5, 2019

@tlinkowski

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Just some feedback: I've been running IntelliJ 2018.3 with a custom-built 0.24 lombok-intellij-plugin (which included this PR) for almost 3 months now (large project at work).

Conclusion: I haven't experienced any problems nor inconveniences (especially no slowdowns that would be noticeable).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.