Skip to content

Commit 8aae78b

Browse files
Hans Halversonfacebook-github-bot
Hans Halverson
authored and
facebook-github-bot
committed
AST nodes with parenthesized type child nodes include child's parens in source location
Summary: The Hermes parser is fixing its handling of source locations around parenthesized types and expressions (choosing to follow the same rules as Flow and Babel). When an expression or type node is parenthesized, the source location of the node correctly does not contain the parentheses. However currently when a parent node has a parenthesized expression or type child node, Hermes is inconsistently including the child's parens in the parent's source location. Hermes should instead always include the child's parentheses in the parent node's source location. For example: ``` (1 + 2) * (3 + 4) ^^^^^ ^^^^^ // Left and right inner expr locs do not contain parens ^^^^^^^^^^^^^^^^^ // Parent binary expression node does contain parens of children ``` We also can now check the end location of the previous token using `getPrevTokenEndLoc`, which will be used to calculate source locations in this scheme instead of building up source locations from child nodes (as child nodes will not contain their own parentheses in their source locations). This diff fixes parenthesized source locations for all Flow type nodes in `JSParserImpl-flow.cpp`. The rules for are as follows: 1. If a node can begin with a type or expression node in the source text, then we must save the start source location before parsing that initial type or expression. This will be the start location of the first paren that wraps that type/expression node, if any exist. 2. If a node can end with a type or expression node in the source text, then we must call `getPrevTokenEndLoc` after the last type/expression node has been parsed. This will be the end location of the first paren that wraps that type/expression node, if any exist. This diff then goes through and implements these rules for every Flow node that can begin or end with a type or expression node. In addition, this diff replaces cases where the end location is explicitly calculated from multiple potential end nodes with a single call to `getPrevTokenEndLocation` at the end of the node. This was done for `GenericTypeAnnotation` nodes and `ClassImplements` nodes. Reviewed By: avp Differential Revision: D25291377 fbshipit-source-id: 35fda8f4e8e92e1197a5e67600bce8ed91e88269
1 parent 95a65af commit 8aae78b

2 files changed

Lines changed: 273 additions & 34 deletions

File tree

‎lib/Parser/JSParserImpl-flow.cpp‎

Lines changed: 30 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,7 @@ Optional<ESTree::Node *> JSParserImpl::parseDeclareFunction(SMLoc start) {
331331
if (!optReturn)
332332
return None;
333333
ESTree::Node *returnType = *optReturn;
334+
SMLoc funcEnd = getPrevTokenEndLoc();
334335

335336
ESTree::Node *predicate = nullptr;
336337
if (check(checksIdent_)) {
@@ -345,10 +346,10 @@ Optional<ESTree::Node *> JSParserImpl::parseDeclareFunction(SMLoc start) {
345346

346347
auto *func = setLocation(
347348
funcStart,
348-
returnType,
349+
funcEnd,
349350
new (context_) ESTree::TypeAnnotationNode(setLocation(
350351
funcStart,
351-
returnType,
352+
funcEnd,
352353
new (context_) ESTree::FunctionTypeAnnotationNode(
353354
std::move(params), returnType, *optRest, typeParams))));
354355
auto *ident = setLocation(
@@ -873,13 +874,14 @@ Optional<ESTree::Node *> JSParserImpl::parseTypeAnnotation(
873874
if (wrappedStart) {
874875
return setLocation(
875876
*wrappedStart,
876-
*optType,
877+
getPrevTokenEndLoc(),
877878
new (context_) ESTree::TypeAnnotationNode(*optType));
878879
}
879880
return *optType;
880881
}
881882

882883
Optional<ESTree::Node *> JSParserImpl::parseUnionTypeAnnotation() {
884+
SMLoc start = tok_->getStartLoc();
883885
checkAndEat(TokenKind::pipe, JSLexer::GrammarContext::Flow);
884886

885887
auto optFirst = parseIntersectionTypeAnnotation();
@@ -901,15 +903,14 @@ Optional<ESTree::Node *> JSParserImpl::parseUnionTypeAnnotation() {
901903
types.push_back(**optInt);
902904
}
903905

904-
SMLoc start = types.front().getStartLoc();
905-
SMLoc end = types.back().getEndLoc();
906906
return setLocation(
907907
start,
908-
end,
908+
getPrevTokenEndLoc(),
909909
new (context_) ESTree::UnionTypeAnnotationNode(std::move(types)));
910910
}
911911

912912
Optional<ESTree::Node *> JSParserImpl::parseIntersectionTypeAnnotation() {
913+
SMLoc start = tok_->getStartLoc();
913914
checkAndEat(TokenKind::amp, JSLexer::GrammarContext::Flow);
914915

915916
auto optFirst = parseAnonFunctionWithoutParensTypeAnnotation();
@@ -931,11 +932,9 @@ Optional<ESTree::Node *> JSParserImpl::parseIntersectionTypeAnnotation() {
931932
types.push_back(**optInt);
932933
}
933934

934-
SMLoc start = types.front().getStartLoc();
935-
SMLoc end = types.back().getEndLoc();
936935
return setLocation(
937936
start,
938-
end,
937+
getPrevTokenEndLoc(),
939938
new (context_) ESTree::IntersectionTypeAnnotationNode(std::move(types)));
940939
}
941940

@@ -974,19 +973,19 @@ Optional<ESTree::Node *> JSParserImpl::parsePrefixTypeAnnotation() {
974973
return None;
975974
return setLocation(
976975
start,
977-
*optPrefix,
976+
getPrevTokenEndLoc(),
978977
new (context_) ESTree::NullableTypeAnnotationNode(*optPrefix));
979978
}
980979
return parsePostfixTypeAnnotation();
981980
}
982981

983982
Optional<ESTree::Node *> JSParserImpl::parsePostfixTypeAnnotation() {
983+
SMLoc start = tok_->getStartLoc();
984984
auto optPrimary = parsePrimaryTypeAnnotation();
985985
if (!optPrimary)
986986
return None;
987987

988988
ESTree::Node *result = *optPrimary;
989-
SMLoc start = result->getStartLoc();
990989

991990
// Parse any [] after the primary type.
992991
while (!lexer_.isNewLineBeforeCurrentToken() &&
@@ -1038,7 +1037,7 @@ Optional<ESTree::Node *> JSParserImpl::parsePrimaryTypeAnnotation() {
10381037
return None;
10391038
return setLocation(
10401039
start,
1041-
*optPrimary,
1040+
getPrevTokenEndLoc(),
10421041
new (context_) ESTree::TypeofTypeAnnotationNode(*optPrimary));
10431042
}
10441043

@@ -1245,7 +1244,7 @@ Optional<ESTree::Node *> JSParserImpl::parseFunctionTypeAnnotationWithParams(
12451244

12461245
return setLocation(
12471246
start,
1248-
*optReturnType,
1247+
getPrevTokenEndLoc(),
12491248
new (context_) ESTree::FunctionTypeAnnotationNode(
12501249
std::move(params), *optReturnType, rest, typeParams));
12511250
}
@@ -1356,7 +1355,7 @@ Optional<ESTree::Node *> JSParserImpl::parseFunctionOrGroupTypeAnnotation() {
13561355
ESTree::Node *typeParams = nullptr;
13571356
return setLocation(
13581357
start,
1359-
*optReturnType,
1358+
getPrevTokenEndLoc(),
13601359
new (context_) ESTree::FunctionTypeAnnotationNode(
13611360
std::move(params), *optReturnType, rest, typeParams));
13621361
}
@@ -1446,7 +1445,7 @@ bool JSParserImpl::parseObjectTypeProperties(
14461445
return false;
14471446
properties.push_back(*setLocation(
14481447
start,
1449-
*optType,
1448+
getPrevTokenEndLoc(),
14501449
new (context_) ESTree::ObjectTypeSpreadPropertyNode(*optType)));
14511450
}
14521451
} else {
@@ -1593,7 +1592,7 @@ bool JSParserImpl::parsePropertyTypeAnnotation(
15931592
assert(value && "value must be set by a branch");
15941593
internalSlots.push_back(*setLocation(
15951594
start,
1596-
value,
1595+
getPrevTokenEndLoc(),
15971596
new (context_) ESTree::ObjectTypeInternalSlotNode(
15981597
id, value, optional, isStatic, method)));
15991598
} else {
@@ -1764,7 +1763,7 @@ Optional<ESTree::Node *> JSParserImpl::parseTypeProperty(
17641763

17651764
return setLocation(
17661765
start,
1767-
value,
1766+
getPrevTokenEndLoc(),
17681767
new (context_) ESTree::ObjectTypePropertyNode(
17691768
key, value, method, optional, isStatic, proto, variance, kind));
17701769
}
@@ -1795,7 +1794,7 @@ Optional<ESTree::Node *> JSParserImpl::parseMethodTypeProperty(
17951794

17961795
return setLocation(
17971796
start,
1798-
value,
1797+
getPrevTokenEndLoc(),
17991798
new (context_) ESTree::ObjectTypePropertyNode(
18001799
key,
18011800
value,
@@ -1836,7 +1835,7 @@ Optional<ESTree::Node *> JSParserImpl::parseGetOrSetTypeProperty(
18361835

18371836
return setLocation(
18381837
start,
1839-
value,
1838+
getPrevTokenEndLoc(),
18401839
new (context_) ESTree::ObjectTypePropertyNode(
18411840
key, value, method, optional, isStatic, proto, variance, kind));
18421841
}
@@ -1898,7 +1897,7 @@ Optional<ESTree::Node *> JSParserImpl::parseTypeIndexerProperty(
18981897

18991898
return setLocation(
19001899
start,
1901-
value,
1900+
getPrevTokenEndLoc(),
19021901
new (context_)
19031902
ESTree::ObjectTypeIndexerNode(id, key, value, isStatic, variance));
19041903
}
@@ -1918,7 +1917,7 @@ Optional<ESTree::Node *> JSParserImpl::parseTypeCallProperty(
19181917
return None;
19191918
return setLocation(
19201919
start,
1921-
*optValue,
1920+
getPrevTokenEndLoc(),
19221921
new (context_) ESTree::ObjectTypeCallPropertyNode(*optValue, isStatic));
19231922
}
19241923

@@ -1969,7 +1968,7 @@ Optional<ESTree::Node *> JSParserImpl::parseTypeParam() {
19691968
if (!need(TokenKind::identifier, "in type parameter", nullptr, {}))
19701969
return None;
19711970
UniqueString *name = tok_->getIdentifier();
1972-
SMLoc end = advance(JSLexer::GrammarContext::Flow).End;
1971+
advance(JSLexer::GrammarContext::Flow);
19731972

19741973
ESTree::Node *bound = nullptr;
19751974
if (check(TokenKind::colon)) {
@@ -1979,9 +1978,8 @@ Optional<ESTree::Node *> JSParserImpl::parseTypeParam() {
19791978
return None;
19801979
bound = setLocation(
19811980
boundStart,
1982-
*optType,
1981+
getPrevTokenEndLoc(),
19831982
new (context_) ESTree::TypeAnnotationNode(*optType));
1984-
end = bound->getEndLoc();
19851983
}
19861984

19871985
ESTree::Node *initializer = nullptr;
@@ -1990,12 +1988,11 @@ Optional<ESTree::Node *> JSParserImpl::parseTypeParam() {
19901988
if (!optInit)
19911989
return None;
19921990
initializer = *optInit;
1993-
end = initializer->getEndLoc();
19941991
}
19951992

19961993
return setLocation(
19971994
start,
1998-
end,
1995+
getPrevTokenEndLoc(),
19991996
new (context_)
20001997
ESTree::TypeParameterNode(name, bound, variance, initializer));
20011998
}
@@ -2057,7 +2054,7 @@ JSParserImpl::parseMethodishTypeAnnotation(
20572054

20582055
return setLocation(
20592056
start,
2060-
*optReturn,
2057+
getPrevTokenEndLoc(),
20612058
new (context_) ESTree::FunctionTypeAnnotationNode(
20622059
std::move(params), *optReturn, *optRest, typeParams));
20632060
}
@@ -2137,7 +2134,7 @@ JSParserImpl::parseFunctionTypeAnnotationParam() {
21372134

21382135
return setLocation(
21392136
start,
2140-
typeAnnotation,
2137+
getPrevTokenEndLoc(),
21412138
new (context_)
21422139
ESTree::FunctionTypeParamNode(name, typeAnnotation, optional));
21432140
}
@@ -2172,19 +2169,17 @@ Optional<ESTree::GenericTypeAnnotationNode *> JSParserImpl::parseGenericType() {
21722169
id, next, new (context_) ESTree::QualifiedTypeIdentifierNode(id, next));
21732170
}
21742171

2175-
SMLoc end = id->getEndLoc();
21762172
ESTree::Node *typeParams = nullptr;
21772173
if (check(TokenKind::less)) {
21782174
auto optTypeParams = parseTypeArgs();
21792175
if (!optTypeParams)
21802176
return None;
21812177
typeParams = *optTypeParams;
2182-
end = typeParams->getEndLoc();
21832178
}
21842179

21852180
return setLocation(
21862181
start,
2187-
end,
2182+
getPrevTokenEndLoc(),
21882183
new (context_) ESTree::GenericTypeAnnotationNode(id, typeParams));
21892184
}
21902185

@@ -2197,19 +2192,20 @@ Optional<ESTree::ClassImplementsNode *> JSParserImpl::parseClassImplements() {
21972192
tok_,
21982193
new (context_)
21992194
ESTree::IdentifierNode(tok_->getIdentifier(), nullptr, false));
2200-
SMLoc end = advance(JSLexer::GrammarContext::Flow).End;
2195+
advance(JSLexer::GrammarContext::Flow);
22012196

22022197
ESTree::Node *typeParams = nullptr;
22032198
if (check(TokenKind::less)) {
22042199
auto optTypeParams = parseTypeArgs();
22052200
if (!optTypeParams)
22062201
return None;
22072202
typeParams = *optTypeParams;
2208-
end = typeParams->getEndLoc();
22092203
}
22102204

22112205
return setLocation(
2212-
start, end, new (context_) ESTree::ClassImplementsNode(id, typeParams));
2206+
start,
2207+
getPrevTokenEndLoc(),
2208+
new (context_) ESTree::ClassImplementsNode(id, typeParams));
22132209
}
22142210

22152211
Optional<ESTree::Node *> JSParserImpl::parsePredicate() {

0 commit comments

Comments
 (0)