Quick fix type mismatch #188

Merged
merged 9 commits into from Oct 3, 2012

6 participants

@kaptoxic

Hi all,

I should have posted this pull request while ago, but now I refactored it a bit so I guess it is ready for pull request.

This is a small pull request, and it provides an implementation of quick fixes when a type missmatch error occurs - e.g. "type mismatch: found Type; required Option[Type]" -> quick fixes suggest to wrap the result in Some().
The implementation is designed to be extensible in a sense that developers can add their own cases of type mismatch and transformations.

For example if one wants to add a quick fix in case of "type mismatch: List[T] but found List[List[T]]" to suggest .flatten, .head or .last call, one could use the add an FoundToRequiredTypeCase object to the cases list which represents all cases of type missmatch errors that should be covered.
Explanation on how to construct such objects is given in the source documentation. The object that does the mentioned quick fix is here.

Some notes on the pull request:

  • Apparently sometimes, the quick fix processor is called multiple times, thus resulting in multiple (identical) quick fixes being offered. Test case:
    def f2(l: Option[Int]) = 4    
    val intVal = 5
    f2(intVal) // <- quick fix here

When two identical (valid) quick fixes are offered. That is an issue of the quick fixe facility itself - should we submit a ticket maybe?

  • Encountered a potential code duplication in the quick fix code. Note that XXX is added at the end of the method name...

Cheers,
Ivan

@dragos
Eclipse Scala IDE member

A general comment: is it possible to add a test for this functionality?

@kaptoxic

Yes, I forgot to mention this. There are no tests that test quick fixes for the Scala IDE currently, if I am not mistaking.

Maybe it would be good to get some feedback from the people who wrote other quick fixes (e.g. class ScalaQuickFixProcessor)?

Of course we could always observe what it is already done for the Eclipse JDT (e.g. AssistQuickFixTest)...

@kaptoxic

By the way, the pr-validator-master-trunk build fails with or without my additions, if I am not mistaking.

@dotta
Eclipse Scala IDE member

I've fixed 2.10 build, so let's give it another spin.

@dotta
Eclipse Scala IDE member

PLS REBUILD ALL

@dotta
Eclipse Scala IDE member

e.g. "type mismatch: found Type; required Option[Type]" -> quick fixes suggest to wrap the result in Some()

Any reason for wrapping with Some? I would have expected Option so that null is mapped to None.

@kaptoxic

Oh, I got the notification about your changes just now... :/ Excellent, the 2.10 build is fine now.

If I understood your remark correctly you suggest that null wraps to None if Option[T] is expected. Isn't it the case that at the place where Option[T] is expected passing null is valid (and does not generate a compiler error marker, i.e. no quick fix functionality is invoked)?

@dragos
Eclipse Scala IDE member

If you pass the null literal, yes, but I don't think that case comes up very often in practice. An expression that has type T may still be null at runtime. In order to correctly make it an Option[T] you need to call Option(e), not Some(e). Some(null) should be avoided at all costs. Option.apply checks the argument for null and returns Some or None accordingly.

@kaptoxic

Yes, I agree, I just wanted to point out that quick fix type mismatch errors are of the form "type mismatch: found A; required B" and the Eclipse quick fix functionality is invoked according to the marker that encapsules such compiler error messages. That should mean that if we encounter a marker with message saying that type T is found while Option[T] is required, wrapping the expression in Some(_) seems appropriate.

Using null value in a similar scenario cannot generate such marker (with such message) and therefore cannot invoke this specific functionality of quick fix type mismatch, therefore offering Some(null) will be avoided (passing null in a place where Option[T] is expected should not invoke the functionality implemented in this pull request).

@dragos
Eclipse Scala IDE member

Sure, we agree on null not generating an error message, therefore there's no way you'd generate Some(null). But I wanted to say something else:

Given an expression e: T, you should not replace it by Some(e), unless you can prove that at runtime e can never be null. Since such an analysis is not performed, you need a runtime test to decide if e is null or not. That is precisely what Option.apply does: it checks the argument for null, and returns Some(e) or None accordingly.

@kaptoxic

Yes, I see now, maybe quick fixes should wrap the expression in Option(_) rather than in Some(_), but in that case the runtime check is payed also in majority of cases where it is not needed. Maybe suggesting both is the best solution?
I guess, given the Scala type system, we cannot be sure whether the value is null or not...

@dragos
Eclipse Scala IDE member

It's ok to suggest Option in all cases. Until non-nullable types appear in Scala, we can't do better.

@ijuma

For those of us that avoid null, using Some seems more appropriate. I like the idea of offering both options. Using Option.apply when no nulls are expected makes things less clear if a null sneaks in.

@dragos
Eclipse Scala IDE member

I'm more fond of the idea that we should be safe. QuickFix is a simple solution to a common problem, and it's probably going to be used by novices. I don't want it to introduce subtle bugs.

The problem with Some is that it does not assert(x nu null), so it silently propagates Some(null) until it blows up later. As someone who's been bitten by Some(null), I definitely avoid Some(expr) like the plague.

One possibility for offering Some as an alternative is to add an assert(e ne null) together with it. Then, experts can delete it consciously.

@ijuma

I understand your concerns. Two points though: quick fixes in JDT are not how you describe them. Since explicit action is required, it doesn't have to be 100% safe (impossible to guarantee anyway). If Option.apply is the first quick fix, people would have to make an effort to choose the second one. The second point is that converting an unexpected null to None does not make errors more obvious.

I agree that it's unfortunate that we don't have a method that fails on nulls before providing a Some.

@dotta
Eclipse Scala IDE member

Guys, where do we stand with this PR?

@kaptoxic

I guess we should decide with which alternative should we go with. Perhaps suggesting both is the best compromise?

@ijuma

I still think we should offer both with Option(..) first. If that's seen as unacceptable, then just the one is better than nothing, I guess. I know I won't ever use it, but others will.

@dotta
Eclipse Scala IDE member

Personally, I'd go with just Option.

@kaptoxic

I agree with @ijuma that users do not have to pay for the check if they are sure (and aware of) what they are doing. What about a comment added to the unsafe proposal?

Well, in either case, I think the changes will not be hard to make, so we just need to decide. :)

@dragos
Eclipse Scala IDE member

Ok, let's add both Some and Option and let the user decide.

@kaptoxic

I added the Option quickfix, now both should be offered.
This was done really easy (the code was designed so that new quickfixes can be easily added) as one can see according to the commit diff.

@kaptoxic

In the last commit, I included a test for quick fixes which I was working on following implementations for the QuickFix JDT tests (e.g. here and here). Unfortunately, it does not pass because calls to JavaCorrectionProcessor (here) does not return any corrections (nor assists).

Any ideas ?

@dragos
Eclipse Scala IDE member

Does it pass when you run it locally, or it never passes?

@kaptoxic

It never passes.
Note that problems are correctly returned from the compiler as expected (here) - they all have expected messages, same as if one highlights the error markers in the Eclipse editor - so I guess that there is a problem with the JavaCorrectionProcessor (perhaps due to the weaving?).

@dragos
Eclipse Scala IDE member

Unfortunately, I don't know what JavaCorrectionProcessor is supposed to do, or how it collects assists. Can't you test it more directly than that, maybe by adding a method in ScalaCompilationUnit to return quick fixes/assists?

@misto misto commented on an outdated diff Oct 2, 2012
...rc/scala/tools/eclipse/quickfix/QuickFixesTests.scala
+ assertTrue("No problems found.", problems.size > 0)
+ assertNumberOfProblems(expectedQuickFixesList.size, problems.toArray)
+
+ // check each problem quickfix
+ for ( (problem, expectedQuickFixes) <- problems zip expectedQuickFixesList) {
+ // here we will accumulate proposals
+ var proposals: ArrayList[IJavaCompletionProposal] = new ArrayList()
+
+ // get all corrections for the problem
+ val offset = problem.getSourceStart
+ val length = problem.getSourceEnd + 1 - offset
+ val context= new AssistContext(unit.getCompilationUnit, offset, length)
+
+ val problemLocation: IProblemLocation = new ProblemLocation(problem);
+ //val status = JavaCorrectionProcessor.collectCorrections(context, Array( problemLocation ), proposals)
+ val status = JavaCorrectionProcessor.collectAssists(context, Array( problemLocation ), proposals)
@misto
Eclipse Scala IDE member
misto added a line comment Oct 2, 2012

collectCorrections should be the correct method to call (assists are for code that doesn't need fixing, like refactoring suggestions)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@misto
Eclipse Scala IDE member

I think I have an idea why the tests don't work. ScalaQuickFixProcessor opens a new editor, and then tries to get the annotations, which doesn't find any. But when I sleep for a few seconds after opening the editor, the annotations are found.. so I guess you somehow need to wait until the editor is fully initialized and all the annotations are available.

@kaptoxic

I thought that JavaCorrectionProcessor is a class for returning corrections (and assists) globally that can be used also for the Scala editor (I was trying to write these test in the correct way by mimicking tests found in JDT testing code).

I've tried using ScalaQuickFixProcessor as you mentioned, but I received null as a result of getCorrections call (when given a problem argument that seems to be valid).

@misto
Eclipse Scala IDE member

In org.eclipse.jdt.inernal.ui.text.correction.JavaCorrectionProcessor, you can see that collectCorrections uses quickFixProcessors contributions and collectAssists uses quickAssistsProcessors. You're right that this alone doesn't solve it, but with collectCorrections and a sleep in ScalaQuickFixProcessor I get the corrections in the tests.

@kaptoxic

Thanks for the explanation! Actually I was trying collectCorrections first but gave a try also to collectAssists just to see what if maybe the results were different (they were not :)).

@misto
Eclipse Scala IDE member

Try this:

--- a/org.scala-ide.sdt.core/src/scala/tools/eclipse/quickfix/ScalaQuickFixProcessor.scala
+++ b/org.scala-ide.sdt.core/src/scala/tools/eclipse/quickfix/ScalaQuickFixProcessor.scala
@@ -62,6 +62,7 @@ class ScalaQuickFixProcessor extends IQuickFixProcessor with HasLogger {
     context.getCompilationUnit match {
       case ssf : ScalaSourceFile => {
        val editor = JavaUI.openInEditor(context.getCompilationUnit)
+       Thread.sleep(5000)
         var corrections : List[IJavaCompletionProposal] = Nil
         for (location <- locations)

then you should get corrections :-)

@kaptoxic

Yes, that seems to be working, thanks again - but is a terrible hack :)))
There are some solutions that can be found online, but I would not burden the "common case" of the QuickFixProcessor... Would you agree?

That's why I've opened an editor and added waiting time into the tests themselves, and the subsequent calls to JavaUI.openInEditor just returned the already opened instance. ScalaQuickFixProcessor can remain unchanged.

I will commit (hopefully working) tests ASAP

@misto
Eclipse Scala IDE member

Oh yes, terrible hack.. it wasn't meant as a solution for the problem, just to point out the issue :-)

@kaptoxic

And it did point out the issue! The tests are working, without changing anything in the core module ;)

P.S. And I guess using the JavaCorrectionProcessor (which uses all those contributions as you mentioned) rather than ScalaQuickFixProcessor directly is the right way to test it - after all, computing proposals directly from the ScalaQuickFixProcessor cannot guarantee us that it will be invoked when the IDE is actually ran.

@kaptoxic

Note that support for wrapping literals into Option is commented at the moment because besides the valid annotation, compiler returns some strange things as annotations (e.g. for 'c' it should return only 'c', but it also returns ').
This quickfix (and tests) are commented.

Other things are working (with respect to small issues mentioned in the description).

@dragos
Eclipse Scala IDE member

LGTM.

@dragos
Eclipse Scala IDE member

BTW, this is not exactly the place to have this discussion, but would it be easy to add a quick fix for misspelled names? For instance, I often mistake the case, or swap a letter in a method name. It would be cool if it showed me what I meant to write. :)

@kaptoxic

Excellent, so this pull request is ready then (but again, note the things mentioned in the description).

Yes, I would say that this should be a similar quick fix which handles errors with messages "value not found: _". Should that be a separate pull request?

@dragos
Eclipse Scala IDE member

Cool, if you feel like contributing that, it should be a separate pull request!

@dragos dragos merged commit c30ce05 into scala-ide:master Oct 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment