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

Error when abstract class is initialized #16

Closed
wants to merge 3 commits into from
Closed

Error when abstract class is initialized #16

wants to merge 3 commits into from

Conversation

wildmaples
Copy link

https://github.com/Shopify/ruby-dev-exp-issues/issues/507

Motivation

We want to add a new error to make sure that abstract! classes are not initialized.

For example:

class Foo
  abstract! 
end

Foo.new # Error!

We don't want a class that inheriting an abstract class to error

class Foo
  abstract! 
end

class Bar < Foo; end

Bar.new # No error

Lastly, we want to make sure singleton methods aren't affected

class Foo
  abstract! 

  def self.hi; end
end

Foo.new # No error

We won't be accounting for receivers that aren't constant literals.

class Foo
  abstract! 
end

x = Foo
x.new # Not covered

Test plan

See included automated tests.

@wildmaples wildmaples requested a review from Morriar July 21, 2022 20:05
@wildmaples wildmaples self-assigned this Jul 21, 2022
Morriar
Morriar reviewed Jul 25, 2022
View reviewed changes
resolver/resolver.cc Outdated
return tree;
}

if (id->symbol == core::Symbols::StubModule()) {
Copy link

Choose a reason for hiding this comment

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

Isn't this already covered by id->symbol.exists() checked earlier?

Copy link
Author

Choose a reason for hiding this comment

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

I don't have a lot of context on stub modules - are stub modules not included in the list of exists()?

Copy link

Choose a reason for hiding this comment

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

I just checked and core::Symbols::StubModule().exists() == true so we may need it.

In which case do we get a StubModule here?

resolver/resolver.cc Show resolved Hide resolved
test/testdata/resolver/abstract_initialization.rb Show resolved Hide resolved
paracycle pushed a commit that referenced this pull request Aug 10, 2022
@jez
Fix crash in go_to_type_definition.cc (sorbet#6135)
21059f0 
* Add a failing test

    + exec test/lsp_test_runner --single_test=test/testdata/lsp/go_to_type_definition.rb
    Pausing
    Resuming
    [doctest] doctest version is "2.4.1"
    [doctest] run with "--help" for options
    [2022-08-01 22:09:09.445] [fatalFallback] [error] Exception::raise(): ./core/TypePtr.h:223 enforced condition store != 0 has failed: (no message provided)

    [2022-08-01 22:09:09.490] [fatalFallback] [error] Backtrace:
      #3 0xc859b2
      #4 0xdbc197 sorbet::core::TypePtr::tag()
      #5 0xef9fe7 sorbet::realmain::lsp::(anonymous namespace)::locsForType()::$_13::operator()()
      #6 0xef95d3 sorbet::typecaseHelper<>()
      #7 0xef8f85 sorbet::typecase<>()
      #8 0xef8b0a sorbet::realmain::lsp::(anonymous namespace)::locsForType()
      #9 0xef859f sorbet::realmain::lsp::TypeDefinitionTask::runRequest()
      #10 0xda699c sorbet::realmain::lsp::LSPRequestTask::run()
      #11 0xf160f4 sorbet::realmain::lsp::(anonymous namespace)::TypecheckerTask::run()
      #12 0xf1454e sorbet::realmain::lsp::LSPTypecheckerCoordinator::asyncRunInternal()
      #13 0xf14620 sorbet::realmain::lsp::LSPTypecheckerCoordinator::syncRun()
      #14 0xf12d44 sorbet::realmain::lsp::LSPLoop::runTask()
      #15 0xf12698 sorbet::realmain::lsp::LSPLoop::processRequests()
      #16 0xf0410d sorbet::realmain::lsp::SingleThreadedLSPWrapper::getLSPResponsesFor()
      #17 0xcbf398 sorbet::test::getLSPResponsesFor()
      #18 0xcbcece sorbet::test::getLSPResponsesFor()
      #19 0xcf1709 sorbet::test::TypeDefAssertion::check()
      #20 0xbbff18 sorbet::test::_DOCTEST_ANON_FUNC_71()
      #21 0x1d060a9 doctest::Context::run()
      #22 0xbc2380 main
      #23 0x7f162d27c083 __libc_start_main
      #24 0xbb34ae _start

    [2022-08-01 22:09:09.496] [fatalFallback] [error] Backtrace:
      #3 0x1d8f6e8 std::terminate()
      #4 0xbcb94b
      #5 0xdbc1c9
      #6 0xef9fe7 sorbet::realmain::lsp::(anonymous namespace)::locsForType()::$_13::operator()()
      #7 0xef95d3 sorbet::typecaseHelper<>()
      #8 0xef8f85 sorbet::typecase<>()
      #9 0xef8b0a sorbet::realmain::lsp::(anonymous namespace)::locsForType()
      #10 0xef859f sorbet::realmain::lsp::TypeDefinitionTask::runRequest()
      #11 0xda699c sorbet::realmain::lsp::LSPRequestTask::run()
      #12 0xf160f4 sorbet::realmain::lsp::(anonymous namespace)::TypecheckerTask::run()
      #13 0xf1454e sorbet::realmain::lsp::LSPTypecheckerCoordinator::asyncRunInternal()
      #14 0xf14620 sorbet::realmain::lsp::LSPTypecheckerCoordinator::syncRun()
      #15 0xf12d44 sorbet::realmain::lsp::LSPLoop::runTask()
      #16 0xf12698 sorbet::realmain::lsp::LSPLoop::processRequests()
      #17 0xf0410d sorbet::realmain::lsp::SingleThreadedLSPWrapper::getLSPResponsesFor()
      #18 0xcbf398 sorbet::test::getLSPResponsesFor()
      #19 0xcbcece sorbet::test::getLSPResponsesFor()
      #20 0xcf1709 sorbet::test::TypeDefAssertion::check()
      #21 0xbbff18 sorbet::test::_DOCTEST_ANON_FUNC_71()
      #22 0x1d060a9 doctest::Context::run()
      #23 0xbc2380 main
      #24 0x7f162d27c083 __libc_start_main
      #25 0xbb34ae _start

    libc++abi: terminate_handler unexpectedly returned
    ===============================================================================
    test/lsp_test_runner.cc:497:
    TEST CASE:  LSPTest

    test/lsp_test_runner.cc:497: FATAL ERROR: test case CRASHED: SIGABRT - Abort (abnormal termination) signal

* Set type to untyped if null
@wildmaples wildmaples force-pushed the error-on-abstract-init branch 2 times, most recently from 101f51a to 7784257 Compare August 16, 2022 16:54
@wildmaples wildmaples marked this pull request as ready for review August 16, 2022 17:01
wildmaples
wildmaples commented Aug 16, 2022
View reviewed changes
resolver/resolver.cc Outdated Show resolved Hide resolved
@wildmaples wildmaples requested a review from a team August 16, 2022 17:05
paracycle
paracycle reviewed Aug 16, 2022
View reviewed changes
resolver/resolver.cc Outdated Show resolved Hide resolved
@vinistock
Copy link
Member

Why do we need a new resolver job? Couldn't we add the error in environment when checking method invocations?

@Morriar
Copy link

Morriar commented Aug 18, 2022

@vinistock we can't base the analysis on types because of this pattern:

class A
  abstract!
end

class B < A; end

sig{ params(a: T.class_of(A)).void }
def takesA(a)
  a.new
end

def assignsA
  a = T.let(B, T.class_of(A))
  a.new 
end

In both cases the recvType would be T.class_of(A) and we would make an error where it should be accepted. The best approach we could think of was to limit the errors to when we spot a .new called on a constant literal that is an abstract class.

Morriar
Morriar approved these changes Aug 18, 2022
View reviewed changes
Copy link

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

I think it's good to be submitted upstream 👍

@wildmaples wildmaples closed this Sep 12, 2022
@wildmaples
Copy link
Author

Ooops forgot to comment I opened a PR upstream (sorbet#6273) so this PR can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paracycle paracycle paracycle left review comments

@dirceu dirceu dirceu left review comments

@Morriar Morriar Morriar approved these changes

Assignees

@wildmaples wildmaples

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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