Skip to content
This repository was archived by the owner on Oct 1, 2019. It is now read-only.

Re-enable AST_COMPARE #17

Merged
merged 4 commits into from
Jan 4, 2018
Merged

Re-enable AST_COMPARE #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a13c306
Re-enable AST_COMPARE
azz Jan 3, 2018
37329f5
Get AST_COMPARE passing
azz Jan 3, 2018
89bb424
Always use source text for numbers
azz Jan 4, 2018
fc87472
Serialize Python numbers to JSON strings, not numbers
azz Jan 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,4 @@ before_install:
- pyenv install -s 3.6.3
- pyenv global 2.7.11 3.6.3
script:
- yarn lint
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @j-f1 The linting is integrated into the Jest run.

- yarn test -- --runInBand
- AST_COMPARE=1 yarn test -- --runInBand
7 changes: 4 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"node": ">=6"
},
"dependencies": {
"prettier": "prettier/prettier#b63c669ed9821a0e0d6918243ef4f4a091b222a9"
"prettier": "prettier/prettier#dbe0758b487be2da74a3f8cf4036472685d75fb0"
},
"devDependencies": {
"eslint": "^4.14.0",
Expand All @@ -26,8 +26,9 @@
"jest-runner-eslint": "^0.3.0"
},
"scripts": {
"lint": "prettier src/**/*.js --list-different",
"test": "jest"
"lint": "jest -c jest.eslint.config.js",
"test": "jest",
"prettier": "prettier --plugin=. --parser=python"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful for testing. yarn prettier test.py or echo 'foo' | yarn prettier

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't need the explicit --parser?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not always, but --debug-check requires it, so it can't hurt.

},
"jest": {
"projects": ["<rootDir>/jest.*.config.js"]
Expand Down
8 changes: 7 additions & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,18 @@ function printComment(commentPath) {
}
}

function clean(ast, newObj) {
delete newObj.lineno;
delete newObj.col_offset;
}

const printers = {
python: {
print,
hasPrettierIgnore: util.hasIgnoreComment,
printComment,
canAttachComment
canAttachComment,
massageAstNode: clean
}
};

Expand Down
18 changes: 10 additions & 8 deletions src/printer/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,12 +297,9 @@ function genericPrint(path, options, print) {
}

case "Num": {
return path.call(print, "n");
}

case "float":
case "int": {
return `${n.n}`;
// This is overly cautious, we may want to revisit and normalize more
// cases here.
return n.source;
}

case "Name": {
Expand Down Expand Up @@ -332,10 +329,15 @@ function genericPrint(path, options, print) {
}

case "Tuple": {
const parent = path.getParentNode();
const needsParens =
["List", "Tuple"].indexOf(path.getParentNode().ast_type) !== -1;
parent.ast_type === "List" || parent.ast_type === "Tuple";
const trailingComma = n.elts.length <= 1;

const elts = join(", ", path.map(print, "elts"));
const elts = concat([
join(", ", path.map(print, "elts")),
trailingComma ? "," : ""
]);

if (needsParens) {
return concat(["(", elts, ")"]);
Expand Down
39 changes: 21 additions & 18 deletions tests/python_expressions/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ exports[`expressions.py 1`] = `
1.8e19
90000000000000
90000000000000.
# 3j
3j

x = a + 1
x = 1 - a
Expand Down Expand Up @@ -85,7 +85,8 @@ l = [i for j in range(10) for i in range(j) if j%2 == 0]
l = [i for j in range(10) for i in range(j) if j%2 == 0 and i%2 == 0]
# l = [(a, b) for (a,b,c) in l2]
# l = [{a:b} for (a,b,c) in l2]
l = [i for j in k if j%2 == 0 if j*2 < 20 for i in j if i%2==0]
# https://github.com/prettier/prettier-python/issues/18
# l = [i for j in k if j%2 == 0 if j*2 < 20 for i in j if i%2==0]

# l = (i for i in j)
# l = (i for i in j if i%2 == 0)
Expand Down Expand Up @@ -341,19 +342,21 @@ a=1;a|=2

-3

24
0x18

1
1.0

3.9

-3.6

18000000000000000000
1.8e19

90000000000000

90000000000000
90000000000000.

3j

x = a + 1

Expand Down Expand Up @@ -383,7 +386,7 @@ x, y, z = 1, 2, 3

l[0]

k[v]
k[v,]

m[a, b]

Expand Down Expand Up @@ -453,8 +456,6 @@ l = [i for j in range(10) for i in range(j) if j % 2 == 0]

l = [i for j in range(10) for i in range(j) if j % 2 == 0 and i % 2 == 0]

l = [i for j in k if j % 2 == 0 j * 2 < 20 for i in j if i % 2 == 0]

l = {a: b,"c": 0}

l = {}
Expand Down Expand Up @@ -719,7 +720,7 @@ exports[`expressions.py 2`] = `
1.8e19
90000000000000
90000000000000.
# 3j
3j

x = a + 1
x = 1 - a
Expand Down Expand Up @@ -789,7 +790,8 @@ l = [i for j in range(10) for i in range(j) if j%2 == 0]
l = [i for j in range(10) for i in range(j) if j%2 == 0 and i%2 == 0]
# l = [(a, b) for (a,b,c) in l2]
# l = [{a:b} for (a,b,c) in l2]
l = [i for j in k if j%2 == 0 if j*2 < 20 for i in j if i%2==0]
# https://github.com/prettier/prettier-python/issues/18
# l = [i for j in k if j%2 == 0 if j*2 < 20 for i in j if i%2==0]

# l = (i for i in j)
# l = (i for i in j if i%2 == 0)
Expand Down Expand Up @@ -1048,22 +1050,22 @@ a=1;a|=2
-3

# 053
24
0x18

# 14L
1
1.0

3.9

-3.6

18000000000000000000
1.8e19

90000000000000

90000000000000
90000000000000.

# 3j
3j

x = a + 1

Expand Down Expand Up @@ -1098,7 +1100,7 @@ x, y, z = 1, 2, 3
# del foo.bar
l[0]

k[v]
k[v,]

m[a, b]

Expand Down Expand Up @@ -1179,7 +1181,8 @@ l = [i for j in range(10) for i in range(j) if j % 2 == 0 and i % 2 == 0]

# l = [(a, b) for (a,b,c) in l2]
# l = [{a:b} for (a,b,c) in l2]
l = [i for j in k if j % 2 == 0 j * 2 < 20 for i in j if i % 2 == 0]
# https://github.com/prettier/prettier-python/issues/18
# l = [i for j in k if j%2 == 0 if j*2 < 20 for i in j if i%2==0]

# l = (i for i in j)
# l = (i for i in j if i%2 == 0)
Expand Down
5 changes: 3 additions & 2 deletions tests/python_expressions/expressions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
1.8e19
90000000000000
90000000000000.
# 3j
3j

x = a + 1
x = 1 - a
Expand Down Expand Up @@ -82,7 +82,8 @@
l = [i for j in range(10) for i in range(j) if j%2 == 0 and i%2 == 0]
# l = [(a, b) for (a,b,c) in l2]
# l = [{a:b} for (a,b,c) in l2]
l = [i for j in k if j%2 == 0 if j*2 < 20 for i in j if i%2==0]
# https://github.com/prettier/prettier-python/issues/18
# l = [i for j in k if j%2 == 0 if j*2 < 20 for i in j if i%2==0]

# l = (i for i in j)
# l = (i for i in j if i%2 == 0)
Expand Down
6 changes: 4 additions & 2 deletions tests_config/run_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const fs = require("fs");
const extname = require("path").extname;
const prettier = require("prettier");
const massageAST = require("prettier/src/common/clean-ast").massageAST;
const normalizeOptions = require("prettier/src/main/options").normalize;

const AST_COMPARE = process.env["AST_COMPARE"];

Expand Down Expand Up @@ -53,15 +54,16 @@ function run_spec(dirname, parsers, options) {

if (AST_COMPARE) {
const ast = parse(source, mergedOptions);
const astMassaged = massageAST(ast);
const normalizedOptions = normalizeOptions(mergedOptions);
const astMassaged = massageAST(ast, normalizedOptions);
let ppastMassaged;
let pperr = null;
try {
const ppast = parse(
prettyprint(source, path, mergedOptions),
mergedOptions
);
ppastMassaged = massageAST(ppast);
ppastMassaged = massageAST(ppast, normalizedOptions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is definitely a value-add, but it's not an ideal API. for purposes of comparison, given a python AST, you can call

ast.dump(tree, annotate_fields=False)

this will give you a string representation of the AST – not the most useful thing for what we're formatting, but you can then just do string equality to compare.

e.g. for this python 2 example:

In [15]: print(s)

with A() as a, B() as b:
    pass


In [16]: print(rt)
with A() as a:
    with B() as b:
        pass


In [17]: print(ast.dump(ast.parse(s), annotate_fields=False))
Module([With(Call(Name('A', Load()), [], [], None, None), Name('a', Store()), [With(Call(Name('B', Load()), [], [], None, None), Name('b', Store()), [Pass()])])])

In [18]: print(ast.dump(ast.parse(rt), annotate_fields=False))
Module([With(Call(Name('A', Load()), [], [], None, None), Name('a', Store()), [With(Call(Name('B', Load()), [], [], None, None), Name('b', Store()), [Pass()])])])

(the two dumped AST strings above are identical)

the difference being in this case that we'd be delegating everything to the ground truth AST handler, such that if there were a bug with the AST -> JSON conversion, we would know

the sort of essential problem from a correctness perspective is that we're not really dealing with a canonical python AST here

funny enough you can't actually assert abstract equality between ASTs in python without dumping them – at not least until python/cpython#1368 lands

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl;dr it would be a better upstream API to have a parseAndMassage hook instead of a massageAST hook. this is because there's a built-in way to canonicalize a python AST to a string that can be used for equality comparisons, but the functionality to convert the python AST to JSON is code under test here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, astor.strip_tree will clean up Python ASTs such that equality checks on them in Python are possible, but I guess there's not a good way to pretty-print the diff for test failures.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nested with case is something we wouldn't change in Prettier for JS. e.g. we would never change:

if (foo) {
  if (bar) {
  }
}

to

if (foo && bar) {
}

There's a reason we massage the AST after we have printed it, as the cleanup often removes properties or nodes that are critical information required to print the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was an illustrative example, showing how this process produces identical dumped ASTs even when the source is quite different in terms of concrete syntax

as it happens, for with, the python 2 AST doesn't distinguish at all between those cases – hence the heuristic in https://github.com/prettier/prettier-python/pull/15/files#diff-ab7972325670f1753462f9b0e6c57900R138

like literally those two things above are identical from the perspective of the parser in python 2 (though not so in python 3)

} catch (e) {
pperr = e.stack;
}
Expand Down
8 changes: 4 additions & 4 deletions vendor/python/astexport.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,18 +87,18 @@ def visit_field_Num_n(self, val):
if isinstance(val, int):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, okay – this should probably just be

def visit_field_Num_n(val):
    return str(val)

this value won't actually be used in the printer (which as with the JS printer will only look at the raw string value), so anything JSON-serializable that will work for the AST comparison will be fine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You lose a bit of information in doing this, does it hurt to keep around the source type?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think you do, actually, relative to what's in the AST:

In [1]: str(1)
Out[1]: '1'

In [2]: str(1.)
Out[2]: '1.0'

In [3]: str(0xDEADBEEF)
Out[3]: '3735928559'

In [4]: str(1_000_001)
Out[4]: '1000001'

In [5]: str(5j)
Out[5]: '5j'

In [6]: str(3-4j)
Out[6]: '(3-4j)'

there might be some edge cases on float rounding, but you'd already encounter them with the above

return {
self.ast_type_field: "int",
"n": val
"n": str(val)
}
elif isinstance(val, float):
return {
self.ast_type_field: "float",
"n": val
"n": str(val)
}
elif isinstance(val, complex):
return {
self.ast_type_field: "complex",
"n": val.real,
"i": val.imag
"n": str(val.real),
"i": str(val.imag)
}


Expand Down
53 changes: 44 additions & 9 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,31 @@
esutils "^2.0.2"
js-tokens "^3.0.0"

"@glimmer/interfaces@^0.30.3":
version "0.30.3"
resolved "https://registry.yarnpkg.com/@glimmer/interfaces/-/interfaces-0.30.3.tgz#ea53e6b945ae3cc14588e655626ad9c6ed90a9f9"
dependencies:
"@glimmer/wire-format" "^0.30.3"

"@glimmer/[email protected]":
version "0.30.3"
resolved "https://registry.yarnpkg.com/@glimmer/syntax/-/syntax-0.30.3.tgz#31ca56d00b4f7c63af13aeb70b5d58a9b250f02e"
dependencies:
"@glimmer/interfaces" "^0.30.3"
"@glimmer/util" "^0.30.3"
handlebars "^4.0.6"
simple-html-tokenizer "^0.4.1"

"@glimmer/util@^0.30.3":
version "0.30.3"
resolved "https://registry.yarnpkg.com/@glimmer/util/-/util-0.30.3.tgz#c771fce2d766c380ae50d3ad8045eff224637e8c"

"@glimmer/wire-format@^0.30.3":
version "0.30.3"
resolved "https://registry.yarnpkg.com/@glimmer/wire-format/-/wire-format-0.30.3.tgz#32629d99f3107b4c940cf84607e417686e55b94d"
dependencies:
"@glimmer/util" "^0.30.3"

"@types/node@*":
version "8.5.2"
resolved "https://registry.yarnpkg.com/@types/node/-/node-8.5.2.tgz#83b8103fa9a2c2e83d78f701a9aa7c9539739aa5"
Expand Down Expand Up @@ -671,6 +696,10 @@ decamelize@^1.0.0, decamelize@^1.1.1:
version "1.2.0"
resolved "https://registry.yarnpkg.com/decamelize/-/decamelize-1.2.0.tgz#f6534d15148269b20352e7bee26f501f9a191290"

[email protected]:
version "0.7.0"
resolved "https://registry.yarnpkg.com/dedent/-/dedent-0.7.0.tgz#2495ddbaf6eb874abb0e1be9df22d2e5a544326c"

deep-extend@~0.4.0:
version "0.4.2"
resolved "https://registry.yarnpkg.com/deep-extend/-/deep-extend-0.4.2.tgz#48b699c27e334bf89f10892be432f6e4c7d34a7f"
Expand Down Expand Up @@ -1289,17 +1318,17 @@ graceful-fs@^4.1.11, graceful-fs@^4.1.2:
version "4.1.11"
resolved "https://registry.yarnpkg.com/graceful-fs/-/graceful-fs-4.1.11.tgz#0e8bdfe4d1ddb8854d64e04ea7c00e2a026e5658"

graphql@0.10.5:
version "0.10.5"
resolved "https://registry.yarnpkg.com/graphql/-/graphql-0.10.5.tgz#c9be17ca2bdfdbd134077ffd9bbaa48b8becd298"
graphql@0.12.3:
version "0.12.3"
resolved "https://registry.yarnpkg.com/graphql/-/graphql-0.12.3.tgz#11668458bbe28261c0dcb6e265f515ba79f6ce07"
dependencies:
iterall "^1.1.0"
iterall "1.1.3"

growly@^1.3.0:
version "1.3.0"
resolved "https://registry.yarnpkg.com/growly/-/growly-1.3.0.tgz#f10748cbe76af964b7c96c93c6bcc28af120c081"

handlebars@^4.0.3:
handlebars@^4.0.3, handlebars@^4.0.6:
version "4.0.11"
resolved "https://registry.yarnpkg.com/handlebars/-/handlebars-4.0.11.tgz#630a35dfe0294bc281edae6ffc5d329fc7982dcc"
dependencies:
Expand Down Expand Up @@ -1706,7 +1735,7 @@ istanbul-reports@^1.1.3:
dependencies:
handlebars "^4.0.3"

iterall@^1.1.0:
[email protected].3:
version "1.1.3"
resolved "https://registry.yarnpkg.com/iterall/-/iterall-1.1.3.tgz#1cbbff96204056dde6656e2ed2e2226d0e6d72c9"

Expand Down Expand Up @@ -2572,17 +2601,19 @@ preserve@^0.2.0:
version "0.2.0"
resolved "https://registry.yarnpkg.com/preserve/-/preserve-0.2.0.tgz#815ed1f6ebc65926f865b310c0713bcb3315ce4b"

prettier@prettier/prettier#b63c669ed9821a0e0d6918243ef4f4a091b222a9:
prettier@prettier/prettier#dbe0758b487be2da74a3f8cf4036472685d75fb0:
version "1.9.2"
resolved "https://codeload.github.com/prettier/prettier/tar.gz/b63c669ed9821a0e0d6918243ef4f4a091b222a9"
resolved "https://codeload.github.com/prettier/prettier/tar.gz/dbe0758b487be2da74a3f8cf4036472685d75fb0"
dependencies:
"@babel/code-frame" "7.0.0-beta.35"
"@glimmer/syntax" "0.30.3"
babylon "7.0.0-beta.34"
camelcase "4.1.0"
chalk "2.1.0"
cjk-regex "1.0.2"
cosmiconfig "3.1.0"
dashify "0.2.2"
dedent "0.7.0"
diff "3.2.0"
editorconfig "0.14.2"
editorconfig-to-prettier "0.0.6"
Expand All @@ -2593,7 +2624,7 @@ prettier@prettier/prettier#b63c669ed9821a0e0d6918243ef4f4a091b222a9:
flow-parser "0.59.0"
get-stream "3.0.0"
globby "6.1.0"
graphql "0.10.5"
graphql "0.12.3"
ignore "3.3.7"
jest-docblock "21.3.0-beta.11"
jest-validate "21.1.0"
Expand Down Expand Up @@ -2947,6 +2978,10 @@ signal-exit@^3.0.0, signal-exit@^3.0.2:
version "3.0.2"
resolved "https://registry.yarnpkg.com/signal-exit/-/signal-exit-3.0.2.tgz#b5fdc08f1287ea1178628e415e25132b73646c6d"

simple-html-tokenizer@^0.4.1:
version "0.4.3"
resolved "https://registry.yarnpkg.com/simple-html-tokenizer/-/simple-html-tokenizer-0.4.3.tgz#9b00b766e30058b4bb377c0d4f97566a13ab1be1"

slash@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/slash/-/slash-1.0.0.tgz#c41f2f6c39fc16d1cd17ad4b5d896114ae470d55"
Expand Down