Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 488531

Summary: [cross file linting] resolve method calls
Product: [ECD] Orion Reporter: Steve Northover <steve_northover>
Component: JS ToolsAssignee: Curtis Windatt <curtis.windatt.public>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: curtis.windatt.public, Olivier_Thomann
Version: unspecified   
Target Milestone: 12.0   
Hardware: PC   
OS: Windows 7   
Whiteboard:
Bug Depends on:    
Bug Blocks: 489691    
Attachments:
Description Flags
WIP
none
Tests none

Description Steve Northover CLA 2016-02-26 03:10:10 EST
Right now cross file linting resolves global functions.  It should also resolve method calls.  If you need an example, I can generate one.

Here is the thinking, "If we can traverse to a method using tern, then we know that the method exists.  If we can offer code assist on the method name while the guy is typing, we must have a list of possible methods. Therefore, somewhere in the system, we know when a method is good and can guess that it is bad"
Comment 1 Curtis Windatt CLA 2016-02-29 09:34:13 EST
I already have a working prototype of this.  I've been tweaking the filtering all last week to try to make it useful but not annoying.  There are many cases where Tern does not have defining type information for what you are working on resulting in your file being littered with errors.
Comment 2 Steve Northover CLA 2016-03-01 16:11:08 EST
What happens when you turn it on for the simple example code?  Can we have it turned off by default, and people can turn it on?
Comment 3 Steve Northover CLA 2016-03-07 14:17:16 EST
What is the status of this feature?  This will be a killer feature for us when/if we can get it working.
Comment 4 Curtis Windatt CLA 2016-03-07 14:53:39 EST
I'm still not 100% happy with the behaviour and there is no fine grained control over what cases are marked as problems (beyond eslint-disable).  Since the rule will be off by default I can still push this into master this week so others can easily try it out.
Comment 5 Steve Northover CLA 2016-03-07 14:56:34 EST
Please do.  I will turn the rule on and try it.
Comment 6 Curtis Windatt CLA 2016-03-08 17:28:37 EST
Created attachment 260166 [details]
WIP

I'm generally happy with the behaviour of this patch, but I found one odd situation due to Tern collecting type information.  In markdownEditor.js when you first lint, you get a number of undeclared members for this._stylerAdapter.<foo>.  If you run openDeclaration on the member expression <foo> it will find a match defined in the same file.  The next time you run linting, we have type information for this._stylerAdapter's properties and don't report any problems.

This is similar to when you visit another file adding its type information to Tern, but happening in a single file.
Comment 7 Steve Northover CLA 2016-03-08 20:19:43 EST
Have you released the code or just attached the patch?  Please don't release for tomorrow unless you are sure the my demo scenario will not break (ie. run it yourself, exactly what I would do, step for step).  Thanks.

I am unbelievably excited about this feature (as you know).
Comment 8 Curtis Windatt CLA 2016-03-09 16:26:21 EST
Created attachment 260194 [details]
Tests

Tests for the new rule.  Includes 2 skipped tests for functionality we should consider implementing/fixing.
1) If a property is later added to a object, Tern doesn't seem to pick up that property.  I think this is the same situation as I saw in markdownEditor where after openDecl Tern then knows about the additional property.
2) If we are seeing a call expression while the type Tern knows about is not a function, we could report a different error ('a' is not a function).

also...
3) I want to experiment with checking all member expressions, not just call expressions.  My first attempts at this created far too many errors, but by filtering to only source objects we have at least some properties for, this might be better now.
Comment 9 Curtis Windatt CLA 2016-03-10 15:58:41 EST
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=5a636ee89471164c43c87762507b8d9ca7167693
Fix with tests

The rule is off by default so it won't affect users.  Turn on Undeclared member expression reference: to try it out.
Comment 10 Steve Northover CLA 2016-03-11 07:21:28 EST
I have turned on the setting and will experiment.

Curtis, please update the wiki to include this and any other new settings we have added.
Comment 11 Steve Northover CLA 2016-03-11 07:28:57 EST
I am likely seeing the same things as you when running on the demo code.  Functions like "getElementById" are not getting resolved despite the fact that I have indicated that I am using the eslint browser environment.  I know that these are early days for this feature.
Comment 12 Curtis Windatt CLA 2016-03-14 10:31:19 EDT
(In reply to Steve Northover from comment #10)
> I have turned on the setting and will experiment.
> 
> Curtis, please update the wiki to include this and any other new settings we
> have added.

Wiki was already updated before I committed :)

(In reply to Steve Northover from comment #11)
> I am likely seeing the same things as you when running on the demo code. 
> Functions like "getElementById" are not getting resolved despite the fact
> that I have indicated that I am using the eslint browser environment.  I
> know that these are early days for this feature.

If running open declaration on getElementID fixes the problem then this is the same issue as me.  If not, it is a Tern environment case we don't handle properly.  I'll investigate this.
Comment 13 Curtis Windatt CLA 2016-03-14 13:07:55 EDT
(In reply to Steve Northover from comment #11)
> I am likely seeing the same things as you when running on the demo code. 
> Functions like "getElementById" are not getting resolved despite the fact
> that I have indicated that I am using the eslint browser environment.  I
> know that these are early days for this feature.

The current rule will not mark errors on any property of document because document doesn't have any known properties (its proto has properties and it has maybeProps).  I can improve this by checking the proto, will push this change to master once I have tests for it.
Comment 14 Curtis Windatt CLA 2016-03-14 14:58:34 EDT
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=7a0859f7a5988ea4a089629bec36fecff4fee084
Rule now warns when using an undeclared member on a object with only proto properties (such as exports.Object).  Tests have been updated.
Comment 15 Curtis Windatt CLA 2016-03-15 17:21:51 EDT
Marking as FIXED.