Skip to content

Simple-minded universal newline normalization - #90

Merged
ilevkivskyi merged 2 commits into
mainfrom
fix-crlf
Sep 9, 2026
Merged

Simple-minded universal newline normalization#90
ilevkivskyi merged 2 commits into
mainfrom
fix-crlf

Conversation

@ilevkivskyi

Copy link
Copy Markdown
Collaborator

It looks like performance effect of this will be small anyway, so we can use a simple-minded solution for the problem described in python/mypy#21823 (comment). One thing I am not sure about is whether to compute hash before or after the normalization. cc @JukkaL

@JukkaL

JukkaL commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

I think it would be better to compute the hash before normalization.

@JukkaL JukkaL left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems good enough, but I think it would be better to calculate the hash for the pre-normalized contents. Otherwise, file hashes will change when updating to a new version of ast_serialize, which seems at least unexpected (though maybe not a big deal).

Also if debugging file hash related issues, it would be nice if the mypy's internal hash function would match this one. I wonder if we rely on mypy's hash function being equivalent to this anywhere?

@ilevkivskyi

Copy link
Copy Markdown
Collaborator Author

I wonder if we rely on mypy's hash function being equivalent to this anywhere?

Yeah, this is exactly my worry as well. Unfortunately I can't really test anything as I don't have a Windows machine, but I will go again through the code and will trust Python docs (e.g. universal newlines should not apply when you read a file as bytes).

@JukkaL

JukkaL commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

When reading a file in binary mode (as bytes), cr/lf is preserved and the bytes should exactly match the file on disk.

@ilevkivskyi

ilevkivskyi commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Sorry, I am being silly, I can simply play with \r (manually added) on Linux, since all the universal newline logic is supposed to be platform independent. I did just that and I think I understand what is going on:

  • We always read files as bytes, so universal newlines do not apply yet.
  • We compute hash immediately after reading file.
  • We then decode the content (universal newlines still do not apply)
  • We then pass the code to ast.parse(), this is where the magic happens, because it applies universal newlines even if one passes in an already decoded string.

I will then update this PR accordingly.

@ilevkivskyi
ilevkivskyi merged commit 30e8c5f into main Sep 9, 2026
25 checks passed
@ilevkivskyi
ilevkivskyi deleted the fix-crlf branch September 9, 2026 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants