Small refactoring of explicitResultTypes #1439
Conversation
| try unsafeFix() | ||
| catch { | ||
| case _: CompilerException => | ||
| shutdownCompiler() |
There was a problem hiding this comment.
The compiler crashes sometimes because it cannot type a part of the AST. If it crashes once, it will crash again even after a retry.
Actually we do that on the file level, which means if one explicit type makes the compiler crash, we don't add any types to the entire file. Should we continue to do that ?
There was a problem hiding this comment.
The compiler crashes sometimes because it cannot type a part of the AST. If it crashes once, it will crash again even after a retry.
Is that the only case? ba74db3 is not very verbose, I am not sure... In any case, if you reproduced the issue, it would be good to document the cases where the compiler crashes and how it behaves via tests.
Actually we do that on the file level, which means if one explicit type makes the compiler crash, we don't add any types to the entire file. Should we continue to do that ?
Catching the exception at a more granular seems like a good idea indeed. Maybe this wasn't done for performance reason because some files would crash the compiler over and over again?
| global.value match { | ||
| case Some(value) => | ||
| val types = new CompilerTypePrinter(value, config) | ||
| ctx.tree.collect { | ||
| case t @ Defn.Val(mods, Pat.Var(name) :: Nil, None, body) | ||
| if isRuleCandidate(t, name, mods, body) => | ||
| fixDefinition(t, body, types) | ||
|
|
||
| case t @ Defn.Var(mods, Pat.Var(name) :: Nil, None, Some(body)) | ||
| if isRuleCandidate(t, name, mods, body) => | ||
| fixDefinition(t, body, types) | ||
|
|
||
| case t @ Defn.Def(mods, name, _, _, None, body) | ||
| if isRuleCandidate(t, name, mods, body) => | ||
| fixDefinition(t, body, types) | ||
| }.asPatch | ||
| case None => Patch.empty | ||
| } |
There was a problem hiding this comment.
old code vs new code:
- in old code, we will always traverse the entire Tree of the file even if we were not able to instantiate the global, which can only happen if the classpath is wrong
- new code: we always instantiate the global, even if there no element (def, or var or val) in the Tree. But in the other hand we remove an indirection where we create a
TypePrinterjust to wrap the case where ScalafixGlobal is None.
There was a problem hiding this comment.
I am not familiar with global, but c6014ce did introduce this for something else than an incorrect classpath it seems
mlachkar
changed the title
|
I will merge this change! I think it's good to simplify the code for explicit result types! |
No description provided.