Protect mkspec against accessing SortedSet without the gem

[trinistr]

Closes #77.

Special handling for SortedSet itself would probably be wrong, as it may genuinely be available, possibly as a gem. Other constants may also raise in the future.

Per suggestion, I tried to reduce scope of rescue, but it made the method very annoying to look at. However, it was rescuing NoMethodError which is not obvious, so I added an extra return if the constant is not a Module.

[trinistr]

Actually, this can be reorganized using rescue-else to preserve nesting. Wow, that's a rare feature to remember.

[trinistr]

Option 1:

  def class_or_module(c)
    const = Object.const_get(c, false)
    return nil unless Module === const

    filtered = @filter && EXCLUDED.include?(const.name)
    return const unless filtered
  rescue NameError, RuntimeError
    # Either the constant doesn't exist or it is
    # explicitly raising an error, like `SortedSet`.
  end

Option 2:

  def class_or_module(c)
    const = Object.const_get(c, false)
  rescue NameError, RuntimeError
    # Either the constant doesn't exist or it is
    # explicitly raising an error, like `SortedSet`.
  else
    return nil unless Module === const

    filtered = @filter && EXCLUDED.include?(const.name)
    return const unless filtered
  end

[eregon]

Option 2 looks OK, but I'm not sure it works on Ruby 2.6 (which mspec still supports for now).
Also some early Ruby implementations might not support such a fancy method-level rescue/else, but this matters less for mkspec as it's meant primarily to be run on CRuby.
I would go with just being simple and explicit:

  def class_or_module(c)
    begin
      const = Object.const_get(c, false)
    rescue NameError, RuntimeError
      # Either the constant doesn't exist or it is
      # explicitly raising an error, like `SortedSet`.
    end
    return nil unless Module === const

    filtered = @filter && EXCLUDED.include?(const.name)
    return const unless filtered
  end

[eregon]

Could you remove this line?
The issue is after triggering the autoload, given that it fails, the constant might be removed or be in a weird state, so it's better to only trigger the autoload once from class_or_module->const_get.

[trinistr]

I completely understand the reservation but I'm reluctant to remove it. Without it the test can easily regress to any of the other cases which return nil.

Actually, there is a test for this exact situtation, so a broken constant would fail the spec.

[eregon]

I completely understand the reservation but I'm reluctant to remove it. Without it the test can easily regress to any of the other cases which return nil.

How could it? The autoload clearly raises the error.

Actually, there is a test for this exact situtation, so a broken constant would fail the spec.

Yes, but that has been the subject of discussions on the CRuby tracker and there have been changes in this area, there used to be "undefined constants" for example.

I suppose one way to meet both of our concerns is to just test both cases, I'll add a second test which doesn't trigger the autoload first.

[trinistr]

How could it? The autoload clearly raises the error.

I'm worried that in the case someone changes the tests, it would be very easy to break this one accidentally. A typo, removing the autoload, or removing raise in the file would all cause the method to return nil, so without the sanity check the test would pass.

Yes, but that has been the subject of discussions on the CRuby tracker and there have been changes in this area, there used to be "undefined constants" for example.

Oh, that's concerning. To be fair, I also intuitively expected that the constant would be in a strange state, but current behavior is clearly superior.

[eregon]

without the sanity check the test would pass.

We could have something like expect(NameMapSpecs.autoload?(:BadFile).to be_a(String) or so, that checks the autoload is registered at least and there is no typo in the constant name.

[eregon]

Thank you for the PR!