Null checks & Issue with resolution of "Unknown.hx" #936
Conversation
… in particular, in HaxePackageModel starting at line 125 the getFile(String fileName){} method was throwing when attempting to resolve "Unknown.hx". This fix is a hack. Where could this request be coming from? To reproduce in the code without this filter, add two macro definitions to the Haxe plugin (js, cpp). Create a haxe class and annotate as follows:
#if js
typedef ArrayBuffer = js.lib.ArrayBuffer;
#else
then change it as follows
#if js
typedef ArrayBuffer = js.lib.ArrayBuffer;
#end
#if cpp
....
#end
| //System.out.println("-----------------------------------"); | ||
| //System.out.println(fileName + ".hx"); | ||
| //System.out.println("-----------------------------------"); | ||
| System.out.println(Arrays.toString(e.getStackTrace())); |
There was a problem hiding this comment.
Always use the HaxeDebugLogger instead of any of the System io dumps (out, err); the System calls can create write dead-locks when multiple JetBrains products are used simultaneously or when called on background threads (for example, see #724). They are also not captured in user logs where they could be useful for debugging.
HaxeDebugLogger.getLogger().warn("Message"); is the easy way to add a temporary message. There are many examples of more interesting usage throughout the code base.
There was a problem hiding this comment.
Cool..thanks for that tip.
| PsiFile file; | ||
| try { | ||
| String fName = fileName + ".hx"; | ||
| //TODO @ Eric. What could be the source of a request for "Unknown.hx" ? |
There was a problem hiding this comment.
There is a magic type of "unknown" in the language that is assigned to variables before type inference is able to set the type. (It's the same in the compiler -- just trace(some_uninitialized_var); to see it pop out.) If the resolver gets a variable in this condition, it's going to try and find the class by upper-casing the name and looking for an implementation file. Were we throwing an exception out of this function? What was the bug that made you look here?
(BTW, ProcessCanceledExceptions are expected and should cancel the current task. That's the way that IntelliJ deals with blocked/blocking tasks.)
There was a problem hiding this comment.
Yeah whenever it tried to load "Unknown.hx" it was throwing. The Intellij stack trace got me there eventually.
There was a problem hiding this comment.
Oh I just noticed your BTW. That may be true but when it threw here it stopped parsing code. This is evident if you can repro from my instructions in the commit notes.
There was a problem hiding this comment.
I've been unable to repro the exception being thrown and parsing stopping. If I set a breakpoint in getFile(), I can definitely see the code trying to find the file -- every time an unknown class reference is created. Issue #938 is for that.
| try { | ||
| String fName = fileName + ".hx"; | ||
| //TODO @ Eric. What could be the source of a request for "Unknown.hx" ? | ||
| if(fName.equals("Unknown.hx")){ |
There was a problem hiding this comment.
BTW, there is nothing in the language that says you can't have a class called Unknown and an Unknown.hx file to go with it. So, just returning null when we see that name is going to create other bugs.
Perhaps we should rename our internal unknown class to __unknown__? The double leading underscore is soft-reserved for compiler internals anyway. Unfortunately, I'm sure that it will pop out to the UI somewhere...
There was a problem hiding this comment.
I agree...I didnt expect my little hack to be permanent.. Shall I try to find that magic type and rename it as you recommended?
|
I changed the merge base to "develop", which is really the current development line. "master" is being updated only upon release at the moment. I might change that, but that's the current M.O.. |
|
I should have said this right at the beginning: Thank You!! for helping out. It's great to have another pair of hands working on the code. |
|
My pleasure. It will take me some time to get up to speed with your substantial code base but I'm invested in this both personally and professionally. Thanks for all your help too. |
|
So do I understand correctly that to resolve this code review my actions are:
|
|
I don't understand an approach inside |
|
Hi @mayakwd . Certainly. When parsing this code #else on line 51 to #end
#if cppusing the latest Haxe plugin from the development branch. I had previously defined macro's in the haxe plugin for both "js" and "cpp" The plugin started to parse the green comment style code in the #else block and then threw an error and stopped. So I checked the stack trace and eventually I eneded up in HaxePackageModel.hx where I discovered that somewhere along the line a request is made to load "Unknown.hx" which does not exist in this project and whenever that request is made it bails. My approach above was to demonstrate the idea and ask for further information, it wasn't intended to be a final solution. Please see if you can reproduce my findings. When I added that crude filter it stopped throwing and completed parsing the code in question. My question is, why is the plugin trying to load "Unknown.hx" when no such file exists? |
|
Hi Wolfie, I missed the notes in the change itself. :/ I need to pay more attention to those... At any rate: Your changes for HaxeResolver.java are good. The changes to HaxeReferenceImpl don't involve any logic change so are also fine. If you submit those as a separate PR, I will merge it. The change to HaxePackageModel, while it stops the problem from occurring, is not the root of the issue. We need to look at the code calling In fact, I just discovered a bug in Also, I'd like to know why looking up a file name is causing parsing to stop, if it's because of anything other than a ProcessCanceledException. If it is consistently stopped by a PCE, we should investigate putting the process on a background thread and/or breaking the process up into smaller chunks that don't overshoot the thread timeouts. Also, there may be an issue with using the virtual file system to find a non-existent file. Finally, I'd like to figure out if the cancellation is causing a bug in the conditional block processing or if it's in the parser itself. (They do interact at parsing time.) |
|
Eric,
Have you tried it with the snow project that I mentioned? It breaks for me
every time when I make that change. I am also on Linux which may make a
difference? I don't know though.
…On Wed, Jul 10, 2019 at 3:59 PM Eric Bishton ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/common/com/intellij/plugins/haxe/model/HaxePackageModel.java
<#936 (comment)>
:
> @@ -125,9 +125,23 @@ public HaxeFileModel getFileModel(String fileName) {
protected HaxeFile getFile(String fileName) {
PsiDirectory directory = root.access(path);
if (directory != null && directory.isValid()) {
- PsiFile file = directory.findFile(fileName + ".hx");
- if (file != null && file.isValid() && file instanceof HaxeFile) {
- return (HaxeFile)file;
+ PsiFile file;
+ try {
+ String fName = fileName + ".hx";
+ //TODO @ Eric. What could be the source of a request for "Unknown.hx" ?
I've been unable to repro the stop exception being thrown. If I set a
breakpoint in getFile(), I can definitely see the code trying to find the
file -- every time an unknown class reference is created. Issue #938
<#938> is for that.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#936?email_source=notifications&email_token=ACSDUBL7WDUZLK5XLAMGBPTP6YWSBA5CNFSM4H7J6PH2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB6B4ORY#discussion_r302223379>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACSDUBPJ3ATANQ7HBPE3VATP6YWSBANCNFSM4H7J6PHQ>
.
|
I have tried it with the content of the file, but not in situ. How did you know that parsing stopped? Was there an error marking or something? |
|
I "knew" because the text that was previously "green" was supposed to
change to highlighted code because I had resolved the condition of my
macro. From "else" to "cpp" which I had added to the plugin. It started
to paint the text with highlighting from the comment green and stopped part
way through and I got the intellij red error bubble with the stack trace.
…On Wed, Jul 10, 2019 at 4:05 PM Eric Bishton ***@***.***> wrote:
Have you tried it with the snow project that I mentioned?
I have tried it with the content of the file, but not *in situ*. How did
you know that parsing stopped? Was there an error marking or something?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#936?email_source=notifications&email_token=ACSDUBPMCD2P7ZTFUZ5KQVLP6YXILA5CNFSM4H7J6PH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZUN3NI#issuecomment-510188981>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACSDUBI5VDBQCZOAOU54GMDP6YXILANCNFSM4H7J6PHQ>
.
|
|
So, I was able to repro in situ. When in the middle of typing, I was getting NPEs., and then after the typing was finished, I had the badly colored text for a bit. I opened "Tools->View PSI of current file" which caused a re-parse and didn't show the error. When I closed the PSI viewer, the file had been reparsed and not showing an error. The NPE report (from idea.log): |
|
Eric,
This is great news in one way (I'm not crazy.. well at least not in regards
to this) and not so good because there is a bug. But I will be happy to
split the PR and take a crack at this later this evening. Very much
appreciate you confirming the bug mate.
/W
…On Wed, Jul 10, 2019 at 4:32 PM Eric Bishton ***@***.***> wrote:
So, I was able to repro *in situ*. When in the middle of typing, I was
getting NPEs., and then after the typing was finished, I had the badly
colored text for a bit. I opened "Tools->View PSI of current file" which
caused a re-parse and didn't show the error. When I closed the PSI viewer,
the file had been reparsed and not showing an error.
The NPE report (from idea.log):
2019-07-10 12:18:41,651 [50711504] ERROR - aemon.impl.PassExecutorService
- IntelliJ IDEA 2018.3.6 Build #IU-183.6156.11
2019-07-10 12:18:41,651 [50711504] ERROR - aemon.impl.PassExecutorService
- JDK: 1.8.0_152-release; VM: OpenJDK 64-Bit Server VM; Vendor: JetBrains
s.r.o
2019-07-10 12:18:41,651 [50711504] ERROR - aemon.impl.PassExecutorService
- OS: Windows 10
2019-07-10 12:18:41,651 [50711504] ERROR - aemon.impl.PassExecutorService
- Last Action: EditorLineStart
2019-07-10 12:18:42,422 [50712275] ERROR - aemon.impl.PassExecutorService
- null
java.lang.NullPointerException
at
com.intellij.plugins.haxe.lang.psi.HaxeResolver.resolveByClassAndSymbol(HaxeResolver.java:433)
at
com.intellij.plugins.haxe.lang.psi.HaxeResolver.doResolve(HaxeResolver.java:122)
at
com.intellij.psi.impl.source.resolve.ResolveCache.lambda$resolve$0(ResolveCache.java:150)
at
com.intellij.openapi.util.RecursionManager$2.doPreventingRecursion(RecursionManager.java:98)
at
com.intellij.psi.impl.source.resolve.ResolveCache.resolve(ResolveCache.java:149)
at
com.intellij.psi.impl.source.resolve.ResolveCache.resolveWithCaching(ResolveCache.java:239)
at
com.intellij.plugins.haxe.lang.psi.HaxeResolver.resolve(HaxeResolver.java:77)
at
com.intellij.plugins.haxe.lang.psi.impl.HaxeReferenceImpl.multiResolve(HaxeReferenceImpl.java:215)
at
com.intellij.plugins.haxe.lang.psi.impl.HaxeReferenceImpl.multiResolve(HaxeReferenceImpl.java:254)
at
com.intellij.plugins.haxe.lang.psi.impl.HaxeReferenceImpl.resolve(HaxeReferenceImpl.java:268)
at
com.intellij.plugins.haxe.lang.psi.impl.HaxeReferenceImpl.resolve(HaxeReferenceImpl.java:167)
at
com.intellij.plugins.haxe.ide.annotator.HaxeAnnotatingVisitor.visitCallExpression(HaxeAnnotatingVisitor.java:86)
at
com.intellij.plugins.haxe.lang.psi.impl.HaxeCallExpressionImpl.accept(HaxeCallExpressionImpl.java:20)
at
com.intellij.psi.impl.PsiElementBase.acceptChildren(PsiElementBase.java:69)
at
com.intellij.plugins.haxe.ide.annotator.HaxeAnnotatingVisitor.visitElement(HaxeAnnotatingVisitor.java:117)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitPsiCompositeElement(HaxeVisitor.java:720)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitExpression(HaxeVisitor.java:171)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitAssignExpression(HaxeVisitor.java:56)
at
com.intellij.plugins.haxe.lang.psi.impl.HaxeAssignExpressionImpl.accept(HaxeAssignExpressionImpl.java:20)
at
com.intellij.psi.impl.PsiElementBase.acceptChildren(PsiElementBase.java:69)
at
com.intellij.plugins.haxe.ide.annotator.HaxeAnnotatingVisitor.visitElement(HaxeAnnotatingVisitor.java:117)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitPsiCompositeElement(HaxeVisitor.java:720)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitBlockStatementPsiMixin(HaxeVisitor.java:636)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitBlockStatement(HaxeVisitor.java:84)
at
com.intellij.plugins.haxe.lang.psi.impl.HaxeBlockStatementImpl.accept(HaxeBlockStatementImpl.java:20)
at
com.intellij.psi.impl.PsiElementBase.acceptChildren(PsiElementBase.java:69)
at
com.intellij.plugins.haxe.ide.annotator.HaxeAnnotatingVisitor.visitElement(HaxeAnnotatingVisitor.java:117)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitPsiCompositeElement(HaxeVisitor.java:720)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitMethod(HaxeVisitor.java:664)
at
com.intellij.plugins.haxe.ide.annotator.HaxeAnnotatingVisitor.visitMethodDeclaration(HaxeAnnotatingVisitor.java:78)
at
com.intellij.plugins.haxe.lang.psi.impl.HaxeMethodDeclarationImpl.accept(HaxeMethodDeclarationImpl.java:20)
at
com.intellij.psi.impl.PsiElementBase.acceptChildren(PsiElementBase.java:69)
at
com.intellij.plugins.haxe.ide.annotator.HaxeAnnotatingVisitor.visitElement(HaxeAnnotatingVisitor.java:117)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitPsiCompositeElement(HaxeVisitor.java:720)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitClassBody(HaxeVisitor.java:110)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitAbstractBody(HaxeVisitor.java:10)
at
com.intellij.plugins.haxe.lang.psi.impl.HaxeAbstractBodyImpl.accept(HaxeAbstractBodyImpl.java:20)
at
com.intellij.psi.impl.PsiElementBase.acceptChildren(PsiElementBase.java:69)
at
com.intellij.plugins.haxe.ide.annotator.HaxeAnnotatingVisitor.visitElement(HaxeAnnotatingVisitor.java:117)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitPsiCompositeElement(HaxeVisitor.java:720)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitClass(HaxeVisitor.java:640)
at
com.intellij.plugins.haxe.lang.psi.HaxeVisitor.visitAbstractClassDeclaration(HaxeVisitor.java:14)
at
com.intellij.plugins.haxe.lang.psi.impl.HaxeAbstractClassDeclarationImpl.accept(HaxeAbstractClassDeclarationImpl.java:20)
at
com.intellij.psi.impl.source.tree.SharedImplUtil.acceptChildren(SharedImplUtil.java:200)
at
com.intellij.psi.impl.source.PsiFileImpl.acceptChildren(PsiFileImpl.java:730)
at
com.intellij.plugins.haxe.ide.annotator.HaxeAnnotatingVisitor.visitElement(HaxeAnnotatingVisitor.java:117)
at com.intellij.psi.PsiElementVisitor.visitFile(PsiElementVisitor.java:34)
at
com.intellij.plugins.haxe.ide.inspections.HaxeUnresolvedSymbolInspection.checkFile(HaxeUnresolvedSymbolInspection.java:84)
at
com.intellij.codeInspection.LocalInspectionTool$1.visitFile(LocalInspectionTool.java:141)
at com.intellij.extapi.psi.PsiFileBase.accept(PsiFileBase.java:70)
at
com.intellij.codeInspection.InspectionEngine.acceptElements(InspectionEngine.java:75)
at
com.intellij.codeInsight.daemon.impl.LocalInspectionsPass.lambda$null$7(LocalInspectionsPass.java:316)
at
com.intellij.util.AstLoadingFilter.lambda$toComputable$2(AstLoadingFilter.java:168)
at
com.intellij.util.AstLoadingFilter.disallowTreeLoading(AstLoadingFilter.java:126)
at
com.intellij.util.AstLoadingFilter.disallowTreeLoading(AstLoadingFilter.java:115)
at
com.intellij.util.AstLoadingFilter.disallowTreeLoading(AstLoadingFilter.java:110)
at
com.intellij.codeInsight.daemon.impl.LocalInspectionsPass.lambda$visitRestElementsAndCleanup$8(LocalInspectionsPass.java:315)
at
com.intellij.concurrency.ApplierCompleter.execAndForkSubTasks(ApplierCompleter.java:133)
at
com.intellij.openapi.application.impl.ApplicationImpl.tryRunReadAction(ApplicationImpl.java:1168)
at
com.intellij.concurrency.ApplierCompleter.lambda$wrapInReadActionAndIndicator$1(ApplierCompleter.java:105)
at
com.intellij.openapi.progress.impl.CoreProgressManager.registerIndicatorAndRun(CoreProgressManager.java:582)
at
com.intellij.openapi.progress.impl.CoreProgressManager.executeProcessUnderProgress(CoreProgressManager.java:532)
at
com.intellij.openapi.progress.impl.ProgressManagerImpl.executeProcessUnderProgress(ProgressManagerImpl.java:87)
at
com.intellij.concurrency.ApplierCompleter.wrapInReadActionAndIndicator(ApplierCompleter.java:116)
at
com.intellij.concurrency.ApplierCompleter.lambda$compute$0(ApplierCompleter.java:96)
at
com.intellij.openapi.application.impl.ReadMostlyRWLock.executeByImpatientReader(ReadMostlyRWLock.java:147)
at
com.intellij.openapi.application.impl.ApplicationImpl.executeByImpatientReader(ApplicationImpl.java:222)
at
com.intellij.concurrency.ApplierCompleter.compute(ApplierCompleter.java:96)
at java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:731)
at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
at
java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
at
java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#936?email_source=notifications&email_token=ACSDUBNDG5NR6E5GHNBQ7G3P6Y2MFA5CNFSM4H7J6PH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZUP4JA#issuecomment-510197284>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACSDUBJ32NLIFPPQAM5KVHLP6Y2MFANCNFSM4H7J6PHQ>
.
|
|
Here's a patch to replace HaxeResolver.java: line 433: final HaxeClass underlyingClass = model.getUnderlyingClass(reference.getSpecialization().toGenericResolver(leftClass));with HaxeGenericSpecialization specialization = reference.getSpecialization();
HaxeGenericResolver resolver = specialization != null ? specialization.toGenericResolver(leftClass) : null;
final HaxeClass underlyingClass = model.getUnderlyingClass(resolver);This fixes the crash that I saw above when I'm running in a heavily modified dev branch. I can't get the NPE to repro using the same project, but a clean dev tree. Since I can't verify that this changes anything on a clean tree, I'm giving up for now. |
Eric,
I thought I would be helpful for a change. Please see the commit notes.