Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop unwrapping types while mapping #585

Merged
merged 11 commits into from
Aug 8, 2024
Merged

Stop unwrapping types while mapping #585

merged 11 commits into from
Aug 8, 2024

Conversation

timholy
Copy link
Member

@timholy timholy commented Aug 4, 2024

This started out as just an attempt to get the remaining tests working on Julia 1.11. However, one of the failures was a case where we wanted to preserve a Core.Const wrapper, and as I dug in it became increasingly clear that it was a little risky to alter the type assigned by inference before mapping.

This PR preserves (or, intends to preserve) the exact types assigned by inference. It then becomes the show component's job to strip Core.Const and similar wrapper before displaying types.

@Zentrik, now the remaining failures seem to be in the vscode portions. Any chance you could take a look at this? I'm less familiar with that code.

@timholy
Copy link
Member Author

timholy commented Aug 4, 2024

Oh wait, it ended up being really simple. The question is whether one thinks 0::Core.Const(0) is a regression compared to 0::Int?

@Zentrik
Copy link
Collaborator

Zentrik commented Aug 4, 2024

Seems like an improvement to me given it's much more specific than Int.

@timholy
Copy link
Member Author

timholy commented Aug 4, 2024

Cool. I'll look into that "exhaustive.jl" failure, and if/when fixed we should have TypedSyntax tests passing on 1.10 and 1.11 (but not nightly).

Given that inference is allowed to change between releases, I don't think any of these changes count as breaking? (Cthulhu is also the only dependent of TypedSyntax listed on juliahub.)

timholy added a commit to timholy/CodeTracking.jl that referenced this pull request Aug 4, 2024
@timholy
Copy link
Member Author

timholy commented Aug 5, 2024

Let's see if CodeTracking PR130 fixed the exhaustive.jl test

@timholy timholy closed this Aug 5, 2024
@timholy timholy reopened this Aug 5, 2024
@timholy
Copy link
Member Author

timholy commented Aug 5, 2024

TypedSyntax is now passing on 1.11. Since this is a fairly significant change and I'm not 100% confident in our test coverage (as good as it is), I'll play with it interactively a little before merging.

@timholy
Copy link
Member Author

timholy commented Aug 8, 2024

This seems to be working reasonably well and without egregious mistakes, so let's merge this.

@timholy timholy merged commit db6c69b into master Aug 8, 2024
17 of 29 checks passed
@timholy timholy deleted the teh/no_unwrap branch August 8, 2024 14:27
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