Bugzilla – Full Text Bug Listing
| Summary: | [quickfix] Fix to remove all unused params, removes too much | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Michael Rennie <Michael_Rennie> |
| Component: | JS Tools | Assignee: | Troy Tao <troytao> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | ||
| Version: | 12.0 | ||
| Target Milestone: | 12.0 | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: |
https://git.eclipse.org/r/69334 https://git.eclipse.org/r/69373 https://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=89421b46be6d795d525ea2b0cc5642927e29ed05 |
||
| Whiteboard: | |||
New Gerrit change created: https://git.eclipse.org/r/69334 After some investigation of the code , it turns out this bug actually is a product of multiple underlying bugs of quickFixes. They are:
1. The way we calculate the start and end positions of the fix obj in removeindexeditemchange (quickfixes.js line 985) is problematic. It ignores how other changes may influence the calculation forms, for example if we start with:
function bar(p1, p2, p3) {
console.log(p1);
}
and we expect that ", p2, p3" should be removed, but the returned obj from removeindexeditemchange only include "p2, p3". It will not remove the trailing comma behind "p1" because this case is ignored by removeindexeditemchange. Moreover, this cannot be solved by simply adding an extra "if" clause because we need the information of other changes to decide whether the comma is an extra one and since we call removeindexeditemchange per change, it does not have that information now. In a case like this:
function bar(p1, p2, p3) {
console.log(p1);
console.log(p2);
}
the comma behind p1 should not be removed.
This problem is tricky to find because due to the second one, if you try with:
function bar(p1, p2, p3) {
console.log(p1);
}
it will correctly remove ", p2, p3" because:
2. The ranges returned from removeindexeditemchange have intersections with each other like this (still use the above example):
{start: 38, end: 42, text: ""}
{start: 40, end: 44, text: ""}
so if we draw a graph:
_ p 2 , _ p 3
38 39 40 41 42
40 41 42 43 44
and after received such ranges, the _modifyContent method in TextView.js (line 6570) starts to process it like this:
first execute {start: 38, end: 42, text: ""}, (notice that it does not cut the character at "start" position):
p 1 , _ p 3
35 36 37 38 39 40 41 42
40 41 42 43 44
and calculate the offset this action caused to other changes: 38 - 42 + "".length = -4, apply this offset to the start and end position of second range (becomes {start: 36, end: 40, text: ""}) and excute it:
p 1
35 36 37 38 39 40 41 42
40 41 42 43 44
The COMBINATION of 1. and 2. problems makes a correct output.
For the reason why Mike's case does not work, let's draw another graph:
(to make it simple, we use a similar code like:
/*eslint-env node */
function bar(p1, p2, p3) {
}
removing the JSDoc part)
First we receive the result ranges from removeindexeditemchange:
{start: 34, end: 38, text: ""}
{start: 38, end: 42, text: ""}
{start: 40, end: 44, text: ""}
draw a graph about the positions of "bar(p1, p2, p3)" and the ranges:
b a r ( p 1 , _ p 2 , _ p 3 )
31 32 33 34 35 36 37 38 39 40 41 42 43 44 45
34 35 36 37 38
38 39 40 41 42
40 41 42 43 44
apply first change {start: 34, end: 38, text: ""}:
b a r ( p 2 , _ p 3 )
31 32 33 34 35 36 37 38 39 40 41 42 43 44 45
38 39 40 41 42
40 41 42 43 44
calculate offset = 0 + start - end + "".length = -4, and apply second change after offset is added {start: 38 - 4, end: 42 - 4, text: ""}:
b a r ( p 3 )
31 32 33 34 35 36 37 38 39 40 41 42 43 44 45
40 41 42 43 44
calculate offset = offset + start - end + "".length = -8, and apply third change after offset is added {start: 40 - 8, end: 44 - 8, text: ""}
b a )
31 32 33 34 35 36 37 38 39 40 41 42 43 44 45
40 41 42 43 44
which is exactly same output as Mike described.
New Gerrit change created: https://git.eclipse.org/r/69373 Gerrit change https://git.eclipse.org/r/69373 was merged to [master]. Commit: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=89421b46be6d795d525ea2b0cc5642927e29ed05 Merged. Thanks Troy! |
Consider the following snippet: /** * @name bar * @description description * @param p1 * @param p2 * @param p3 * @returns returns */ function bar(p1, p2, p3) { } use 'fix all' to remove p1, p2 and p3. After the fix we wind up with: /** * @name bar * @description description * @returns returns */ function ba) { } Using the fix one at a time works properly.