Quick fix for non UTF-8 encodings with new parser - #22066
ilevkivskyi wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the PR, it seems quite likely that some older projects may still have some files with non-UTF-8 encodings lying around.
| if source is None: | ||
| # Try reading/encoding manually, since native parser only supports UTF-8. | ||
| # If we still cannot decode, decode error will buble up to caller. | ||
| with open(filename, "rb") as f: |
There was a problem hiding this comment.
If there is a file read error, ast_serialize may raise a ValueError. This means that this could raise (e.g. consider file which the user has no read access to).
There was a problem hiding this comment.
OK, yeah, I will catch OSError in build.py (essentially matching what old parser does).
| # Try reading/encoding manually, since native parser only supports UTF-8. | ||
| # If we still cannot decode, decode error will buble up to caller. | ||
| with open(filename, "rb") as f: | ||
| source = decode_python_encoding(f.read()) |
There was a problem hiding this comment.
I think the non-UTF-8 fallback may break incremental caching, as the hash is inconsistently used (original latin 1 hash vs re-encoded utf-8 hash). Add an incremental mode test case that ensurer that caching works. This could be kind of bad, if a single latin-1 file in a big code base could break incremental mode.
There was a problem hiding this comment.
Good catch (this will affect only cases where mtime has changed, but it is still good to handle).
There was a problem hiding this comment.
Oh LOL, we already have the same problem with the old parser as well. I will try to fix that one too.
There was a problem hiding this comment.
Actually nvm, that was only for -c code path, which is irrelevant, since we never actually write cache for it.
| ) | ||
| else: | ||
| # Convert everything unexpected to a standard-looking blocker. | ||
| raise CompileError([f"{filename}: error: Cannot parse file: {exc}"]) from exc |
There was a problem hiding this comment.
Should we just re-raise instead here? This seems to override logic in parse_file_inner with a less specific error message.
There was a problem hiding this comment.
No, I don't want to simply re-raise since then I will need to catch ValueError in build.py, which is too broad and may mask genuine bugs in the future.
| except ValueError as exc: | ||
| if source is None: | ||
| # Try reading/encoding manually, since native parser only supports UTF-8. | ||
| # If we still cannot decode, decode error will buble up to caller. |
| source = "# coding: ascii\nJérôme = False" | ||
| with temp_source(source, encoding="latin1") as fnam: | ||
| with pytest.raises(UnicodeDecodeError): | ||
| parse_to_binary_ast(fnam, Options()) |
There was a problem hiding this comment.
Add also more end-to-end tests in check-...test (also incremental, as suggested in another comment). Maybe also add at least one daemon test, just in case?
There was a problem hiding this comment.
OK, fine, I was too lazy to write them, I will add an incremental test for the above.
|
Diff from mypy_primer, showing the effect of this PR on open source code: schema_salad (https://github.com/common-workflow-language/schema_salad)
- mypy: error: Cannot read file 'schema_salad': No such file or directory
+ schema_salad: error: Cannot read file: No such file or directory
|
|
@ilevkivskyi is this ready to merge? so that we can cherry pick and mention it in the changelog |
3 participants
Fixes #22055
As discussed in the issue. This is not super-principled, but it works, and also gives nice standard colorized blocker if the fallback doesn't work either.
cc @JukkaL