Improve ActiveSupport::Dependencies code understanding

[aaronang]

Summary

In pull request #24198, I removed all log-related stuff in ActiveSupport::Dependencies as this was no longer useful. However, some of the log messages were still useful to understand the code. Therefore, in this patch, those messages are put back as comments.

Other Information

Please have a look at pull request #24198 as reference.

Looking forward to hearing from you 😄

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[maclover7]

r? @fxn

[fxn]

There's something strange here, already present before we removed the traces: the comment says an error condition is going on if you reached this point. This should be obvious from reading the source.

I think this could be rewritten to be more clear. Instead of aborting let's use a flag with the opposite meaning like success. Then we can just say:

if success
  new_constants
else
  # Remove partially loaded constants.
  new_constants.each { |c| remove_constant(c) }.clear
end

The intention to reverse the flag is to have the optimistic path first in the if/else.

[aaronang]

@fxn Thank you for your elaborate review, really appreciated! Just pushed a commit that incorporates your feedback 😬

[fxn]

Could still be simpler (just iterating over this already existing piece of code since we are on it, also with an intention to go through a little refactor since you are students and it may be interesting to see these kind of arguments).

Why does it need a return? because there is a fallback outside the ensure that ends up returning an empty array. But a fallback to fall through if there is an error following a different code path than the happy path? And why do we clear a local variable no longer used? Let's do another pass!

What about (written right here):

def new_constants_in(*descs)
  constant_watch_stack.watch_namespaces(descs)
  success = false

  begin
    yield # Now yield to the code that is to define new constants.
    success = true
  ensure
    new_constants = constant_watch_stack.new_constants

    if success
      new_constants
    else
      # Remove partially loaded constants.
      new_constants.each { |c| remove_constant(c) }
      []
    end
  end
end

Note the return is gone, that is an idiomatic if/else in Ruby that leverages these are expressions.

Also removed the clear call. What is the point of modifying in place a local variable that is of no use? Technically it gives us the same value we wanna return, but it communicates to the reader an intention we do not have and they have to think and maybe check the docs (I did) to understand the call happens to return the modified self. What we really want to do is two separate things 1) remove partially loaded constants, and 2) return an empty array.

[aaronang]

@fxn Thanks again for your in-depth explanation.

I agree with you on removing the clear call as it can be confusing.

I am in no way an experienced Ruby user. However, if I understand correctly #new_constants_in is expected to return a list; it can be an empty or non-empty list. The problem with the proposed approach is that it will fallback to true, as ensure clauses have no effect on what is returned (unless you return something explicitly). Am I understanding this correctly?

[fxn]

Aahhh, you're totally right! Was typing off the top of my head and mislead by the final array.

Let me come back with a different refactor.

[fxn]

Here's another rewrite:

def new_constants_in(*descs)
  constant_watch_stack.watch_namespaces(descs)

  begin
    yield # Now yield to the code that is to define new constants.
  rescue Exception
    # Remove partially loaded constants.
    constant_watch_stack.new_constants.each do |new_constant|
      remove_constant(new_constant)
    end
    raise
  else
    constant_watch_stack.new_constants
  end
end

I think it more straightforward than the current

def new_constants_in(*descs)
  constant_watch_stack.watch_namespaces(descs)
  aborting = true

  begin
    yield # Now yield to the code that is to define new constants.
    aborting = false
  ensure
    new_constants = constant_watch_stack.new_constants

    return new_constants unless aborting

    new_constants.each { |c| remove_constant(c) }.clear
  end

  []
end

I bet the intention of the original ensure block is to ensure constant_watch_stack.new_constants is called no matter what, but I don't think the cost in clarity pays off. As a hint that the code was not obvious to follow, it has had an unreachable empty array for years!

[matthewd]

I think we need the ensure; it's the only block that will be run for some forms of stack unwind.

[fxn]

@matthewd which workflow would work on the current code and not the refactored one?

[matthewd]

Non-exception stack unwinds: throws and non-local returns are the ones that come to mind.

[fxn]

@matthewd but are you talking about the above implementation, or thinking about an eventual refactor in which a return is inserted for example?

[matthewd]

We don't control the contents of the block we're yielding to. Okay, a non-local return would be tricky to achieve in a top-level file context, but a throw is perfectly possible.

[fxn]

I was not aware that ensure is executed if there is a throw.

Can't see it in Flanagan's or Ruby docs, I find it a bit surprising ensure is described in terms of exceptions and throwing formally is not raising an exception. But it definitely has a spec and we need to program against how things work anyway.

Both implementations are not equivalent then.

[fxn]

Added coverage for that situation, the test in 52ce6ec passes with the current implementation and fails with the alternative above.

[fxn]

I have worked a little bit more on this method.

There is now test coverage for both raising and throwing, and includes checking for autoloading recursion.

Then I did some project archeology and saw the origin or the unreachable line with the array literal, see the commit message of 4b80395.

Then removed the clear call in bf6b654.

@aaronang needs a rebase then. After all this thinking about this method I believe we would only reverse the flag. I prefer to read the positive/optimistic

return new_constants if success

than

return new_constants unless aborting

In this case I also prefer the return call there instead of the if/else, because that return is important and has to be prominent.

[aaronang]

@fxn Incorporated your feedback 😉 It's cool to see how you discuss things so elaborate.

Unfortunately, Travis CI failed in the Action Cable module. Is there anything I can do about this?

[fxn]

@aaronang don't worry, the patch seems correct, passed locally right?

[fxn]

@aaronang just in case I mean the AS suite, no need to run Action Cable's.

[aaronang]

@fxn The Active Support test suite passes locally 😉

[fxn]

Awesome, in then, thanks a lot! ❤️

[vm00z]

❤️ 💎

[aaronang]

@fxn Feel free to approach us again with other tasks 😬

[fxn]

@aaronang @Valmai awesome, I'll write you people back soon with another idea.