Remove the concept of globalPrecedingNode (#561)

The original motivation for this change is trying to fix #560 where the comment was attached to the JSXText node instead of the JSXExpressionContainer because it used the globalPrecedingNode. In general, I don't think that it is very safe to attach a comment to a random node just because it happened to be before.

I tried to delete the globalPrecedingNode codepath and I think that it actually improves the results. I'll add inline comments in the pull request to explain the various changes.

Fixes #560
This commit is contained in:
Christopher Chedeau authored and GitHub committed 2017-02-03 13:56:06 -08:00
1 parent 0924e7f3dc
commit 3b66af3d60
25 files changed
+320 -193

No files matched your search

+12 -16
View File
@@ -86,13 +86,6 @@ function decorateComment(node, comment, text) {
// The comment is completely contained by this child node.
comment.enclosingNode = child;
if (middle > 0) {
// Store the global preceding node separately from
// `precedingNode` because they mean different things. This is
// the previous node, ignoring any delimiters/blocks/etc.
comment.globalPrecedingNode = childNodes[middle - 1];
}
decorateComment(child, comment, text);
return; // Abandon the binary search at this level.
}
@@ -140,7 +133,6 @@ function attach(comments, ast, text) {
decorateComment(ast, comment, text);
const precedingNode = comment.precedingNode;
const globalPrecedingNode = comment.globalPrecedingNode;
const enclosingNode = comment.enclosingNode;
const followingNode = comment.followingNode;
const isStartOfFile = comment.loc.start.line === 1;
@@ -169,15 +161,16 @@ function attach(comments, ast, text) {
}
} else if (util.hasNewline(text, locEnd(comment))) {
// There is content before this comment on the same line, but
// none after it, so prefer a trailing comment. A trailing
// comment *always* attaches itself to the previous node, no
// matter if there's any syntax between them.
const lastNode = precedingNode || globalPrecedingNode;
if (lastNode) {
// Always a trailing comment
addTrailingComment(lastNode, comment);
// none after it, so prefer a trailing comment of the previous node.
if (precedingNode) {
addTrailingComment(precedingNode, comment);
} else if (followingNode) {
addLeadingComment(followingNode, comment);
} else if (enclosingNode) {
addDanglingComment(enclosingNode, comment);
} else {
throw new Error("Preceding node not found");
// TODO: If there are no nodes at all, we should still somehow
// print the comment.
}
} else {
// Otherwise, text exists both before and after the comment on
@@ -200,6 +193,9 @@ function attach(comments, ast, text) {
addLeadingComment(followingNode, comment);
} else if (enclosingNode) {
addDanglingComment(enclosingNode, comment);
} else {
// TODO: If there are no nodes at all, we should still somehow
// print the comment.
}
}
});
+4 -11
View File
@@ -1206,20 +1206,13 @@ function genericPrintNoParens(path, options, print) {
path.call(print, "identifier")
]);
case "ClassBody":
if (n.body.length === 0) {
return group(
concat([
"{",
comments.printDanglingComments(path, options),
softline,
"}"
])
);
if (!n.comments && n.body.length === 0) {
return "{}";
}
return concat([
"{",
indent(
n.body.length > 0 ? indent(
options.tabWidth,
concat([
hardline,
@@ -1230,7 +1223,7 @@ function genericPrintNoParens(path, options, print) {
"body"
)
])
),
) : comments.printDanglingComments(path, options),
hardline,
"}"
]);
+6
View File
@@ -0,0 +1,6 @@
class A {
// comment
}
class A { // comment
}
@@ -46,7 +46,8 @@ class A {
// comment
}
class A {} // comment
class A {// comment
}
class A {}
"
@@ -615,3 +615,135 @@ foo(
);
"
`;
exports[`test jsx.js 1`] = `
"<div>
{/* comment */
}
</div>;
<div>
{/* comment
*/
}
</div>;
<div>
{a/* comment
*/
}
</div>;
<div>
{/* comment
*/
a
}
</div>;
<div>
{/* comment */
}
</div>;
<div>
{/* comment */}
</div>;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<div>
{/* comment */}
</div>;
<div>
{/* comment
*/}
</div>;
<div>
{a /* comment
*/}
</div>;
<div>
{
/* comment
*/
a
}
</div>;
<div>
{/* comment */}
</div>;
<div>
{/* comment */}
</div>;
"
`;
exports[`test jsx.js 2`] = `
"<div>
{/* comment */
}
</div>;
<div>
{/* comment
*/
}
</div>;
<div>
{a/* comment
*/
}
</div>;
<div>
{/* comment
*/
a
}
</div>;
<div>
{/* comment */
}
</div>;
<div>
{/* comment */}
</div>;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<div>
{/* comment */}
</div>;
<div>
{/* comment
*/}
</div>;
<div>
{a /* comment
*/}
</div>;
<div>
{
/* comment
*/
a
}
</div>;
<div>
{/* comment */}
</div>;
<div>
{/* comment */}
</div>;
"
`;
+32
View File
@@ -0,0 +1,32 @@
<div>
{/* comment */
}
</div>;
<div>
{/* comment
*/
}
</div>;
<div>
{a/* comment
*/
}
</div>;
<div>
{/* comment
*/
a
}
</div>;
<div>
{/* comment */
}
</div>;
<div>
{/* comment */}
</div>;
@@ -345,7 +345,8 @@ function test2() {
//
function test3() {
async function voidoid4(): Promise<void> { // ok
async function voidoid4(): Promise<void> {
// ok
console.log(\"HEY\");
}
}
@@ -356,13 +357,15 @@ function test3() {
//
function test4() {
async function voidoid5(): void { // error, void != Promise<void>
async function voidoid5(): void {
// error, void != Promise<void>
console.log(\"HEY\");
}
}
function test5() {
async function voidoid6(): Promise<number> { // error, number != void
async function voidoid6(): Promise<number> {
// error, number != void
console.log(\"HEY\");
}
}
@@ -178,7 +178,8 @@ var alias1: Alias = new _Alias(); // error: bad pun
var alias2: Alias = _Alias.factory(); // error: bad pun
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
var Bar = class Foo {
static factory(): Foo { // OK: Foo is a type in this scope
static factory(): Foo {
// OK: Foo is a type in this scope
return new Foo(); // OK: Foo is a runtime binding in this scope
}
};
@@ -43,7 +43,8 @@ function c(): number {
return temp ? temp : 0;
}
function d(): string { // expected \`: number | boolean\`
function d(): string {
// expected \`: number | boolean\`
// equivalent to \`return x != null && x\`
var x: ?number = null;
return x != null ? x : x != null;
@@ -458,27 +458,23 @@ function tup_pattern2<X>([proj]: Proj<X>) {} // proj: X
function rest_antipattern<T>(...t: T) {} // nonsense
function rest_pattern<X>(...r: X[]) {} // r: X[]
function obj_rest_pattern<X>(
{ _, ...o }: { _: any, x: X } // o: { x: X }
) {
function obj_rest_pattern<X>({ _, ...o }: { _: any, x: X }) {
// o: { x: X }
o.x;
}
type ObjRest<X> = { _: any, x: X };
function obj_rest_pattern<X>(
{ _, ...o }: ObjRest<X> // o: { x: X }
) {
function obj_rest_pattern<X>({ _, ...o }: ObjRest<X>) {
// o: { x: X }
o.x;
}
function arr_rest_pattern<X>(
[_, ...a]: [any, X] // a: [X]
) {
function arr_rest_pattern<X>([_, ...a]: [any, X]) {
// a: [X]
a[0];
}
type ArrRest<X> = [any, X];
function arr_rest_pattern<X>(
[_, ...a]: ArrRest<X> // a: [X]
) {
function arr_rest_pattern<X>([_, ...a]: ArrRest<X>) {
// a: [X]
a[0];
}
"
@@ -633,21 +633,18 @@ function subtype_dict_to_optional_c(x: { [k: string]: B }) {
let c: { p?: C } = x; // error
}
function subtype_optional_a_to_dict(
x: { p?: A }
): { [k: string]: B } { // error: A ~> B
function subtype_optional_a_to_dict(x: { p?: A }): { [k: string]: B } {
// error: A ~> B
return x;
}
function subtype_optional_b_to_dict(
x: { p?: B }
): { [k: string]: B } { // ok
function subtype_optional_b_to_dict(x: { p?: B }): { [k: string]: B } {
// ok
return x;
}
function subtype_optional_c_to_dict(
x: { p?: C }
): { [k: string]: B } { // error: C ~> B
function subtype_optional_c_to_dict(x: { p?: C }): { [k: string]: B } {
// error: C ~> B
return x;
}
"
@@ -157,9 +157,8 @@ function makeIterator(coin_flip: () => boolean): Iterator<string> {
},
next(): IteratorResult<string, void> {
var done = coin_flip();
if (
done // Whoops, made a mistake and forgot to negate done
) {
if (done) {
// Whoops, made a mistake and forgot to negate done
return { done, value: \"still going...\" }; // Error string ~> void
} else {
return { done }; // Error void ~> string
@@ -519,7 +519,8 @@ function logical19b(x: { y: string, z: boolean }): boolean {
/**
* A falsy variable on the left side of &&
*/
function logical1a(): number { // expected \`: boolean\`
function logical1a(): number {
// expected \`: boolean\`
var x = false;
return x && \"123\";
}
@@ -535,7 +536,8 @@ function logical1b(): string {
/**
* A literal on the left side of &&
*/
function logical2a(): number { // expected \`: boolean\`
function logical2a(): number {
// expected \`: boolean\`
return false && \"123\";
}
@@ -612,7 +614,8 @@ function logical2k(x: Function): string {
/**
* An expression on the left side of &&
*/
function logical3a(): string { // expected \`: boolean\`
function logical3a(): string {
// expected \`: boolean\`
var x: ?number = null;
return x != null && x > 10;
}
@@ -620,7 +623,8 @@ function logical3a(): string { // expected \`: boolean\`
/**
* An expression on the left side of &&
*/
function logical3b(): number { // expected \`: boolean | number\`
function logical3b(): number {
// expected \`: boolean | number\`
var x: ?number = null;
return x != null && x;
}
@@ -628,7 +632,8 @@ function logical3b(): number { // expected \`: boolean | number\`
/**
* An expression on the left side of &&
*/
function logical3c(): ?number { // expected \`: boolean | ?number\`
function logical3c(): ?number {
// expected \`: boolean | ?number\`
var x: ?number = null;
return x != undefined && x;
}
@@ -636,9 +641,8 @@ function logical3c(): ?number { // expected \`: boolean | ?number\`
/**
* Maybe truthy returns both types
*/
function logical4(
x: boolean
): string { // expected \`: boolean | string\`
function logical4(x: boolean): string {
// expected \`: boolean | string\`
return x && \"123\";
}
@@ -661,7 +665,8 @@ function logical5b(): number {
/**
* A truthy variable on the left side of ||
*/
function logical5c(): string { // expected \`: boolean\`
function logical5c(): string {
// expected \`: boolean\`
var x = true;
return x || 0;
}
@@ -690,7 +695,8 @@ function logical6c(): number {
/**
* A literal on the left side of ||
*/
function logical6d(): number { // expected \`: boolean\`
function logical6d(): number {
// expected \`: boolean\`
return true || \"123\";
}
@@ -794,7 +800,8 @@ function logical8e(): string {
/**
* A composite || and &&
*/
function logical8f(): string { // expected \`: boolean\`
function logical8f(): string {
// expected \`: boolean\`
var x = true;
return x || 1 && \"foo\";
}
@@ -802,10 +809,8 @@ function logical8f(): string { // expected \`: boolean\`
/**
* A composite || and ||
*/
function logical9a(
x: number,
y: string
): number | string { // expected \`: number | string | boolean\`
function logical9a(x: number, y: string): number | string {
// expected \`: number | string | boolean\`
return x || y || false;
}
@@ -826,30 +831,24 @@ function logical9c(x: number, y: boolean): string {
/**
* A composite && and &&
*/
function logical10a(
x: number,
y: string
): number | string { // expected \`: number | string | boolean\`
function logical10a(x: number, y: string): number | string {
// expected \`: number | string | boolean\`
return x && y && false;
}
/**
* A composite && and &&
*/
function logical10b(
x: number,
y: string
): Array<any> { // expected \`: boolean\`
function logical10b(x: number, y: string): Array<any> {
// expected \`: boolean\`
return false && x && y;
}
/**
* A composite && and &&
*/
function logical10c(
x: number,
y: string
): Array<any> { // expected \`number | boolean\`
function logical10c(x: number, y: string): Array<any> {
// expected \`number | boolean\`
return x && false && y;
}
@@ -91,21 +91,18 @@ module.exports = Foo, Bar;
/* @flow */
var Foo = {
a: function(
arg // missing arg annotation
) {
a: function(arg) {
// missing arg annotation
return arg;
},
b: function(
arg // missing arg annotation
) {
b: function(arg) {
// missing arg annotation
return {
bar: arg
};
},
c: function(
arg: string // no return annotation required
) {
c: function(arg: string) {
// no return annotation required
return {
bar: arg
};
@@ -123,17 +120,15 @@ var Foo = {
// observed return type (e.g. param types in a function type) must come
// from annotations
e: function(arg: string) {
return function(
x // missing param annotation
) {
return function(x) {
// missing param annotation
return x;
};
},
// ...if the return type is annotated explicitly, this is unnecessary
f: function(arg: string): (x: number) => number {
return function(
x // no error
) {
return function(x) {
// no error
return x;
};
}
@@ -101,7 +101,7 @@ function goofy() {
var x = g();
if (typeof x == \"function\") {
x();
} else { // if (typeof x == \'number\') {
} else {// if (typeof x == \'number\') {
//f(x);
}
}
@@ -177,30 +177,30 @@ type T1 = {};
type T2 = { x: number };
type T3 = { x: number, y: number };
class C1
extends React.Component<T1, T2, any> {} // error
class C1 extends React.Component<T1, T2, any> {// error
}
class C2
extends React.Component<void, T2, any> {} // OK
class C2 extends React.Component<void, T2, any> {// OK
}
// no need to add type arguments to React.Component
class C3
extends React.Component { // OK
class C3 extends React.Component {
// OK
static defaultProps: T1;
props: T2;
}
class C4
extends React.Component { // OK, recommended
class C4 extends React.Component {
// OK, recommended
// no need to declare defaultProps unless necessary
props: T2;
}
class C5
extends React.Component<T2, T3, any> {} // error
class C5 extends React.Component<T2, T3, any> {// error
}
class C6
extends React.Component { // OK, recommended
class C6 extends React.Component {
// OK, recommended
static defaultProps: T2;
props: T3;
}
@@ -15,13 +15,13 @@ class A<X> {}
new A(); // OK, implicitly inferred type args
class B extends A {} // OK, same as above
function foo(
b
): A<any> { // ok but unsafe, caller may assume any type arg
function foo(b): A<any> {
// ok but unsafe, caller may assume any type arg
return b ? (new A(): A<number>) : (new A(): A<string>);
}
function bar(): A<*> { // error, * can\'t be {} and {x: string} at the same time
function bar(): A<*> {
// error, * can\'t be {} and {x: string} at the same time
return (new A(): A<{}>) || (new A(): A<{ x: string }>);
}
"
@@ -451,9 +451,8 @@ function foo(x: string | Array<string>): string {
// Sanity check:
// - predicate functions cannot have bodies (can only be declarations)
function pred(
x: mixed
): boolean %checks(typeof x === \"string\") { // error: cannot use pred type here
function pred(x: mixed): boolean %checks(typeof x === \"string\") {
// error: cannot use pred type here
return typeof x === \"string\";
}
@@ -37,9 +37,8 @@ var a2 = (x: mixed): %checks (x !== null) => { // Error: body form
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// @flow
var a2 = (
x: mixed // Error: body form
): %checks(x !== null) => {
var a2 = (x: mixed): %checks(x !== null) => {
// Error: body form
var x = 1;
return x;
};
@@ -47,17 +47,15 @@ type Bar = {
};
function bar0(x: Bar) {
while (
x = x.parent // can\'t assign x to ?Bar
) {
while (x = x.parent) {
// can\'t assign x to ?Bar
x.doStuff();
}
}
function bar1(x: ?Bar) {
while (
x = x.parent // x.parent might be null
) {
while (x = x.parent) {
// x.parent might be null
x.doStuff();
}
}
@@ -758,17 +756,15 @@ function foo8(o: { p: mixed }) {
// @flow
function foo1(o: { x: number }) {
if (
o.p1 // OK, this is an idiomatic way of testing property existence
) {
if (o.p1) {
// OK, this is an idiomatic way of testing property existence
o.x;
}
}
function foo2(o: { x: number }) {
if (
o.p2 // OK
) {
if (o.p2) {
// OK
o.p2.x; // error, since o.p2\'s type is unknown (e.g., could be \`number\`)
}
}
@@ -778,9 +774,8 @@ function foo3(o: { x: number }) {
}
function foo4(o: $Exact<{ x: number }>) {
if (
o.p4 // OK
) {
if (o.p4) {
// OK
o.p4.x; // currently OK, should be unreachable
} else {
o.p4.x; // error
@@ -1399,15 +1394,11 @@ let tests = [
}
},
function(num: number, obj: { foo: number }) {
if (
num === obj.bar // ok, typos allowed in conditionals
) {
if (num === obj.bar) {// ok, typos allowed in conditionals
}
},
function(num: number, obj: { [key: string]: number }) {
if (
num === obj.bar // ok
) {
if (num === obj.bar) {// ok
}
},
function(n: number): Mode {
@@ -1983,15 +1974,11 @@ let tests = [
}
},
function(str: string, obj: { foo: string }) {
if (
str === obj.bar // ok, typos allowed in conditionals
) {
if (str === obj.bar) {// ok, typos allowed in conditionals
}
},
function(str: string, obj: { [key: string]: string }) {
if (
str === obj.bar // ok
) {
if (str === obj.bar) {// ok
}
},
function(str: string): Mode {
@@ -2080,9 +2067,8 @@ class A {
class B {
test(): string {
if (
super.prop // super.prop doesn\'t exist
) {
if (super.prop) {
// super.prop doesn\'t exist
return super.prop; // error, unknown type passed to string expected
}
return \"B\";
@@ -2537,9 +2523,8 @@ function nationality(x: Citizen | NonCitizen) {
let tests = [
// non-existent props
function test7(x: A) {
if (
x.kindTypo === 1 // typos are allowed to be tested
) {
if (x.kindTypo === 1) {
// typos are allowed to be tested
(x.kindTypo: string); // typos can\'t be used, though
}
},
@@ -2569,53 +2554,46 @@ let tests = [
) {
if (x.length === 0) {
}
if (
x.legnth === 0 // typos are allowed to be tested
) {
if (x.legnth === 0) {
// typos are allowed to be tested
(x.legnth: number); // inside the block, it\'s a number
(x.legnth: string); // error: number literal 0 !~> string
}
if (y.length === 0) {
}
if (
y.legnth === 0 // typos are allowed to be tested
) {
if (y.legnth === 0) {
// typos are allowed to be tested
(y.legnth: number); // inside the block, it\'s a number
(y.legnth: string); // error: number literal 0 !~> string
}
if (z.toString === 0) {
}
if (
z.toStirng === 0 // typos are allowed to be tested
) {
if (z.toStirng === 0) {
// typos are allowed to be tested
(z.toStirng: number); // inside the block, it\'s a number
(z.toStirng: string); // error: number literal 0 !~> string
}
if (q.valueOf === 0) {
}
if (
q.valeuOf === 0 // typos are allowed to be tested
) {
if (q.valeuOf === 0) {
// typos are allowed to be tested
(q.valeuOf: number); // inside the block, it\'s a number
(q.valeuOf: string); // error: number literal 0 !~> string
}
if (
r.toStirng === 0 // typos are allowed to be tested
) {
if (r.toStirng === 0) {
// typos are allowed to be tested
(r.toStirng: empty); // props on AnyObjT are \`any\`
}
if (s.call === 0) {
}
if (
s.calll === 0 // typos are allowed to be tested
) {
if (s.calll === 0) {
// typos are allowed to be tested
(t.calll: empty); // ok, props on functions are \`any\` :/
}
if (t.call === 0) {
}
if (
t.calll === 0 // typos are allowed to be tested
) {
if (t.calll === 0) {
// typos are allowed to be tested
(t.calll: empty); // ok, props on functions are \`any\` :/
}
},
@@ -2871,9 +2849,8 @@ function anyobj(x: number | Object): number {
}
function testInvalidValue(x: mixed) {
if (
typeof x === \"foo\" // error
) {
if (typeof x === \"foo\") {
// error
return 0;
}
}
@@ -2885,9 +2862,8 @@ function testTemplateLiteral(x: string | number) {
}
function testInvalidTemplateLiteral(x: string | number) {
if (
typeof x === \`foo\` // error
) {
if (typeof x === \`foo\`) {
// error
return 0;
}
}
@@ -183,7 +183,8 @@ class E {
foo(x: number) {}
}
class F extends E {
foo() { // OK to override with generalization
foo() {
// OK to override with generalization
(() => {
super.foo(\"\"); // find super method, error due to incorrect arg
})();
@@ -357,7 +357,8 @@ class B extends A { } // inherits statics method too, with \`this\` bound to the
// supporting \`this\` type in statics
class A {
static make(): this { // factory method, whose return type \`this\` (still)
static make(): this {
// factory method, whose return type \`this\` (still)
// describes instances of A or subclasses of A: the
// meaning of the \`this\` type is not changed simply by
// switching into a static context
@@ -554,9 +554,7 @@ function bar(response) {
throw new Error(\"...\");
}
// here via [try] only.
if (
payload.error // ok
) {
if (payload.error) {// ok
// ...
}
}
@@ -126,14 +126,15 @@ interface MyInterface<T> {
x: T
}
interface MySubinterface extends MyInterface { // no error
interface MySubinterface extends MyInterface {
// no error
y: number
}
// no arity error in extends of polymorphic class
class MySubclass
extends MyClass { // ok, type arg inferred
class MySubclass extends MyClass {
// ok, type arg inferred
y: number;
constructor(y: number) {
super(y);
@@ -124,14 +124,15 @@ interface MyInterface<T> {
x: T
}
interface MySubinterface extends MyInterface { // error, missing argument list
interface MySubinterface extends MyInterface {
// error, missing argument list
y: number
}
// *no* arity error in extends of polymorphic class
class MySubclass
extends MyClass { // ok, type arg inferred
class MySubclass extends MyClass {
// ok, type arg inferred
y: number;
constructor(y: number) {
super(y);