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.

Sign up for GitHub

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

Jump to bottom

Fix backwards compatibility with sorbet-runtime #1277

Merged
merged 3 commits into from
Dec 19, 2022
Merged

Fix backwards compatibility with sorbet-runtime #1277

merged 3 commits into from
Dec 19, 2022

Conversation

dirceu
Copy link
Contributor

@dirceu dirceu commented Nov 21, 2022

This reintroduces the monkeypatch to T::Utils#coerce that was removed on #1270. See https://github.com/Shopify/tapioca/pull/1270/files#r1026916745 for more information.

@dirceu dirceu requested a review from a team as a code owner November 21, 2022 13:36
@paracycle
Copy link
Member

Also, we will need a test with an older version of Sorbet to make sure we don't break old behaviour.

@dirceu
Copy link
Contributor Author

dirceu commented Nov 21, 2022

we will need a test with an older version of Sorbet to make sure we don't break old behaviour.

Are there other examples of a test like this, so I use a similar setup?

@paracycle
Copy link
Member

Are there other examples of a test like this, so I use a similar setup?

It was recently removed: 76515f5#diff-bee8bd10954cc2dcd94f010d4bc1b3092af1883d0494cfda0c1b653718550b11

But, basically a gem CLI test that sets up Tapioca against both an older and a newer version of the sorbet-runtime gem (on both sides of the breaking upstream PR) to test the codepath that would be broken if we got this wrong (either) way is tested properly.

@dirceu
Copy link
Contributor Author

dirceu commented Nov 24, 2022

In order to test this we had to make exe/tapioca stop suppressing typechecking errors coming from gem/application files it requires.

Interestingly, if we remove both versions of *CoercePatch and continue suppressing errors, the generated RBI file is exactly the same as it would be with the patches present.

The only test that breaks if both patches are removed is this one, which fails when the file is required by the Tapioca process that runs tests (not by exe/tapioca: so without suppressing errors).

Now:

  1. If Tapioca can generate the same RBIs with and without those patches, do we need those patches in the first place? Are there other tests we might be missing?
  2. Should we enforce typechecking on other CLI tests as well, or would it be better to just continue emulating user behaviour (i.e. run exe/tapioca without modifications, suppressing typechecking errors)? If we stop suppressing errors in exe/tapioca for all tests, we get 8 failures and 1 error. I didn't dig into whether they're real bugs or just test setup quirks, but it's not too much stuff to fix if we want to make this change.

@dirceu dirceu self-assigned this Nov 28, 2022
dirceu added a commit that referenced this pull request Nov 29, 2022
@dirceu
Avoid swallowing errors in (most) CLI tests
3e31b06 
By default `exe/tapioca` will ignore many errors, including typechecking
ones. This is because we want to generate as much RBI as possible, even
if there are errors in the codebase.

We do want to surface more errors in the CLI tests, however, so this PR
is making CLI tests throw by default, while providing an option to
ignore it explicitly in cases where the updates to the test setup would
not be worth the hassle.

See #1277 (comment)
@dirceu dirceu mentioned this pull request Nov 30, 2022
Merged
@dirceu dirceu removed their assignment Dec 2, 2022
Morriar pushed a commit that referenced this pull request Dec 8, 2022
@dirceu @Morriar
Avoid swallowing errors in (most) CLI tests
c166591 
By default `exe/tapioca` will ignore many errors, including typechecking
ones. This is because we want to generate as much RBI as possible, even
if there are errors in the codebase.

We do want to surface more errors in the CLI tests, however, so this PR
is making CLI tests throw by default, while providing an option to
ignore it explicitly in cases where the updates to the test setup would
not be worth the hassle.

See #1277 (comment)
@Morriar Morriar self-assigned this Dec 8, 2022
Morriar pushed a commit that referenced this pull request Dec 8, 2022
@dirceu @Morriar
Avoid swallowing errors in (most) CLI tests
12c42a1 
By default `exe/tapioca` will ignore many errors, including typechecking
ones. This is because we want to generate as much RBI as possible, even
if there are errors in the codebase.

We do want to surface more errors in the CLI tests, however, so this PR
is making CLI tests throw by default, while providing an option to
ignore it explicitly in cases where the updates to the test setup would
not be worth the hassle.

See #1277 (comment)
@Morriar Morriar assigned paracycle and unassigned Morriar Dec 12, 2022
Morriar pushed a commit that referenced this pull request Dec 12, 2022
@dirceu @Morriar
Avoid swallowing errors in (most) CLI tests
5822808 
By default `exe/tapioca` will ignore many errors, including typechecking
ones. This is because we want to generate as much RBI as possible, even
if there are errors in the codebase.

We do want to surface more errors in the CLI tests, however, so this PR
is making CLI tests throw by default, while providing an option to
ignore it explicitly in cases where the updates to the test setup would
not be worth the hassle.

See #1277 (comment)
@dirceu dirceu requested review from vinistock and a team December 19, 2022 15:15
dirceu and others added 3 commits December 19, 2022 18:31
@dirceu @paracycle
Stop returning names of TypeVariableModule instances
4bdb19e 
Sorbet runtime should never need to look up the name of a
`TypeVariableModule` instance, since that type should always be coerced
to a `Tapioca::TypeVariable` instance by the point the `name` method is
called.

The fact that we were doing this was redundant and was preventing us
from detecting instances where a `TypeVariableModule` instance was not
being properly coerced to a `Tapioca::TypeVariable` instance.
@dirceu @Morriar @paracycle
Add backwards-compatibility test
1adec87 
Co-authored-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@dirceu @paracycle
Fix backwards compatibility with sorbet-runtime
ea2039a 
This reintroduces the monkeypatch to T::Utils#coerce that was removed
on #1270. See
https://github.com/Shopify/tapioca/pull/1270/files#r1026916745 for more
information.
@paracycle
Copy link
Member

Ok, I've verified that the change in the T::Types::Simple::GenericPatch#name method does not lead to any typechecking errors in Core, either in gem command nor dsl command output.

I'd also verified that both tests fail if their corresponding module doesn't do the patching correctly, so this PR should be ready to go.

@dirceu dirceu merged commit 5139e04 into main Dec 19, 2022
@dirceu dirceu deleted the fix-backwards-compatibility branch December 19, 2022 20:10
@shopify-shipit shopify-shipit bot temporarily deployed to production December 19, 2022 22:14 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Morriar Morriar Morriar left review comments

@paracycle paracycle paracycle left review comments

@vinistock vinistock vinistock approved these changes

Assignees

@paracycle paracycle

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dirceu @paracycle @Morriar @vinistock