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

Sorbet provides confusing output (or crashes) if a constant is named T #3133

Closed
mistydemeo opened this issue Jun 2, 2020 · 12 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@mistydemeo
Copy link

mistydemeo commented Jun 2, 2020

Input

This doesn't reproduce in the same way on sorbet.run.

Any assignment to T causes this; for example, T = "blah" is enough.

Observed output

Several thousand errors which resemble this:

sorbet/rbi/sorbet-typed/lib/bundler/all/bundler.rbi:8100: Malformed type declaration. Unknown type syntax. Expected a ClassName or T.<func> https://srb.help/5004
    8100 |      options: T.untyped,
                         ^^^^^^^^^

In sorbet.run, the error message is completely different and much easier to understand:

editor.rb:2: Redefining constant T https://srb.help/4012
     2 |T = "blah"
        ^^^^^^^^^^
    https://github.com/sorbet/sorbet/tree/master/rbi/sorbet/t.rbi#L16: Previous definition
    16 |module T
        ^^^^^^^^
Errors: 1

Expected behavior

A few things would be expected:

  1. Definition of T in code being typechecked doesn't affect Sorbet's own definition of T, allowing the typechecking to simply work.
  2. The user is given a more specific message.
  3. One, precise message (for example, Redefining constant T) instead of several thousand confusing messages.

This can be reproduced by running srb tc -e 'T = "blah"'.

In at least one moderately-sized application (Homebrew), where Sorbet inadvertently ran on our dependencies, a dependency's spec helper containing the statement T = Regexp::Syntax::Token caused Sorbet to crash with a SIGSTOP instead of reporting errors. Discussion in Slack with @trevor-stripe brought up that this looks like a call to ENFORCE that shouldn't be terminating the process in a release build of Sorbet.

I haven't been able to isolate the repro step outside Homebrew's own codebase. To repro, you'll need to run the following steps:

  1. git clone https://github.com/mistydemeo/brew.git
  2. cd brew && git checkout reproduce_sorbet_crash
  3. Ensure you're running Ruby 2.6.3 with the version manager of your choice.
  4. cd Library/Homebrew
  5. bundle install
  6. bundle exec srb tc
@mistydemeo mistydemeo added bug Something isn't working unconfirmed This issue has not yet been confirmed by the Sorbet team labels Jun 2, 2020
@mistydemeo
Copy link
Author

Here's the backtrace for the crash:

* thread #1, stop reason = signal SIGSTOP
  * frame #0: 0x000000010049da96 sorbet`sorbet::ast::ConstantLit::fullUnresolvedPath(sorbet::core::GlobalState const&) const + 150
    frame #1: 0x0000000100336f9f sorbet`sorbet::resolver::(anonymous namespace)::ResolveConstantsWalk::resolveAncestorJob(sorbet::core::MutableContext, sorbet::resolver::(anonymous namespace)::ResolveConstantsWalk::AncestorResolutionItem&, bool) + 1663
    frame #2: 0x00000001003344da sorbet`sorbet::resolver::(anonymous namespace)::ResolveConstantsWalk::resolveConstants(sorbet::core::GlobalState&, std::__1::vector<sorbet::ast::ParsedFile, std::__1::allocator<sorbet::ast::ParsedFile> >, sorbet::WorkerPool&) + 10330
    frame #3: 0x000000010033140c sorbet`sorbet::resolver::Resolver::run(sorbet::core::GlobalState&, std::__1::vector<sorbet::ast::ParsedFile, std::__1::allocator<sorbet::ast::ParsedFile> >, sorbet::WorkerPool&) + 92
    frame #4: 0x0000000100147d46 sorbet`sorbet::realmain::pipeline::resolve(std::__1::unique_ptr<sorbet::core::GlobalState, std::__1::default_delete<sorbet::core::GlobalState> >&, std::__1::vector<sorbet::ast::ParsedFile, std::__1::allocator<sorbet::ast::ParsedFile> >, sorbet::realmain::options::Options const&, sorbet::WorkerPool&, bool) + 1094
    frame #5: 0x000000010000678f sorbet`sorbet::realmain::realmain(int, char**) + 7311
    frame #6: 0x00000001000023cb sorbet`main + 11
    frame #7: 0x00007fff72371cc9 libdyld.dylib`start + 1

@elliottt elliottt removed the unconfirmed This issue has not yet been confirmed by the Sorbet team label Jun 2, 2020
@vaporyhumo

This comment has been minimized.

@mistydemeo
Copy link
Author

I'm not myself calling T; it's Sorbet's typechecker that's having issues.

@vaporyhumo

This comment has been minimized.

@mistydemeo
Copy link
Author

An of course, if you try to redefine root level T that's gonna conflict with sorbet, since you are trying to redefine it's module.

Yes, but Sorbet shouldn't be crashing! A single informative message would also help here, instead of roughly 4000 confusing messages.

@vaporyhumo

This comment has been minimized.

@vaporyhumo

This comment has been minimized.

@elliottt
Copy link
Contributor

elliottt commented Jun 2, 2020

Using a debug build, I'm hitting an earlier ENFORCE in the namer:

ENFORCE(existing.exists());

@jvilk-stripe
Copy link
Collaborator

Thank you for the report! I probably caused this when I rearchitected namer.

@vaporyhumo as @mistydemeo states, this is clearly a bug in Sorbet and not intended behavior! Sorbet should never crash.

@jvilk-stripe jvilk-stripe self-assigned this Jun 2, 2020
@vaporyhumo

This comment has been minimized.

@vaporyhumo
Copy link
Contributor

I'm deleting my old comments since they add nothing of value to an open issue.

@elliottt
Copy link
Contributor

elliottt commented May 7, 2021

I re-ran the following program on a debug build, and no longer notice an ENFORCE failing:

# typed: true

T = "blah"

@elliottt elliottt closed this as completed May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants