search

typetools / checker-framework

Pluggable type-checking for Java
http://checkerframework.org/
Other
990 stars 347 forks source link

FlowExpressionParser replacement #2634

Closed Maxi17 closed 4 years ago

Maxi17 commented 4 years ago

This version works correctly but it's not complete. Every parseX method can be optimized or removed. I added comments to explain the changes.

Double literals can now be parsed. Fixes #1614

smillst commented 4 years ago

Is this a fix for https://github.com/typetools/checker-framework/issues/1614?

Maxi17 commented 4 years ago

Is this a fix for #1614?

Yes. Sorry for omitting it. I updated the comment.

Maxi17 commented 4 years ago

I also made some small performance improvements that are not related to the JavaParser replacement. Hopefully these won't be a problem.

Maxi17 commented 4 years ago

I tried to remove matchPackageAndClassNameWithinExpression. It performed the job of a symbol solver, so the change I made is just a way around it. Anyways, I think this method and other helper methods I deleted at the latest commit are necessary because they throw exceptions with useful and descriptive messages. My 3-line version of them, found in parseClassExpression, doesn't.

I will revert to the previous commit if you think this change isn't good enough.

smillst commented 4 years ago

@Maxi17 said:

I tried to remove matchPackageAndClassNameWithinExpression. It performed the job of a symbol solver, so the change I made is just a way around it. Anyways, I think this method and other helper methods I deleted at the latest commit are necessary because they throw exceptions with useful and descriptive messages. My 3-line version of them, found in parseClassExpression, doesn't.

I think you should remove the matchPackageAndClassNameWithinExpression method. The method(s) you replace it with should do something similar , only using JavaParser expressions instead of Strings. I've added a comment to get you started.

Maxi17 commented 4 years ago

I made the changes. If the expression is a ClassExpr, then it either goes in parseIdentifier (in case of String.class) or parseMemberSelect (in case of java.lang.String.class) after the parseHelper call. Both parseIdentifier and parseMemberSelect work correctly. I haven't added the exception handling of the old matchPackageAndClassNameWithinExpression.

Maxi17 commented 4 years ago

Some methods still need Javadoc. I will write them. The test I added is superficial, but it only needs to prove that parsing floats/doubles is now permitted.

Maxi17 commented 4 years ago

@kelloggm @panacekcz @smillst I hope I added everything. If the Javadoc or the ParsingDouble test case needs improvement, please let me know (or you can update them directly). Otherwise, I think this is the final version of the replacement.

Maxi17 commented 4 years ago

Supporting char literals is simple. Supporting String concatenation and mathematical operators can be done using BinaryExpr. Something else that can be supported is ObjectCreationExpr, for #1635.

smillst commented 4 years ago

Supporting char literals is simple. Supporting String concatenation and mathematical operators can be done using BinaryExpr. Something else that can be supported is ObjectCreationExpr, for #1635.

Great! Please add support for char literals in this pull request. Once this pull request is merged, then you can work on add support for the other Expressions.

Maxi17 commented 4 years ago

@smillst I made the changes. There are still some problems:

  1. I had to declare the ProcessingEnvironment and Types variables in many methods, instead of just once. I think there must be a better way for this, but in the constructor I can't access the FlowExpressionContext variable passed as argument.

  2. For now, I made FlowExpressionParseException an unchecked exception, waiting for something better.

  3. Char literals are now permitted, so I will add a test case.

  4. I removed unhelpful Javadoc, but haven't added any new.

smillst commented 4 years ago
  1. I had to declare the ProcessingEnvironment and Types variables in many methods, instead of just once. I think there must be a better way for this, but in the constructor I can't access the FlowExpressionContext variable passed as argument.

You can pass the ProcessingEnvironment to the constructor, then uses fields.

  1. For now, I made FlowExpressionParseException an unchecked exception, waiting for something better.

Sounds good. You can figure that out later. (I added a TODO so I don't merge that change into master.)

Maxi17 commented 4 years ago

@smillst I finished it. The checked exception is now wrapped into an unchecked exception inside the class.

Maxi17 commented 4 years ago

Also have you tested this branch on any of the Index Checker case studies?

@smillst No, I haven't. But I would appreciate some tips on how to do it.

mernst commented 4 years ago

A paper about the Index Checker case studies is here: https://homes.cs.washington.edu/~mernst/pubs/array-indexing-issta2018.pdf The three main case studies in that paper are:

I have given the repositories for the initial work and where the annotations belong. I have not checked whether the annotations have been moved; if not, could you please move them?

smillst commented 4 years ago

@Maxi17 Mike pointed out where the Index Checker case studies are. (@kelloggm and @panacekcz can help with building them.) I would like you to run each one with typetools/master and this branch and then compare time. I'm only looking for them to take roughly the same amount of time.

Maxi17 commented 4 years ago

@mernst @smillst I have run the Guava case study and plume-lib case study on both master and issue1614v3 branches multiple times. Running mvn process-sources on Guava, the results were roughly the same (on average, 1:02 minutes for both branches, on my machine). For plume-lib, running make produced, again, similar results between branches (on average, 38 seconds). Each run of these 2 case studies took either 1-2 seconds longer or 1-2 seconds shorter, but there was no visible evidence to support one implementation.

For JFreeChart, the annotations are still on a PR.