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

Improve Rails' Shape friendliness (second pass) #47032

Merged
merged 2 commits into from
Jan 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 13 additions & 5 deletions actionpack/lib/action_controller/metal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,17 @@ def self.action_encoding_template(action) # :nodoc:
false
end

class << self
private
def inherited(subclass)
super
subclass.middleware_stack = middleware_stack.dup
subclass.class_eval do
@controller_name = nil
end
end
end

# Delegates to the class's ::controller_name.
def controller_name
self.class.controller_name
Expand Down Expand Up @@ -166,7 +177,9 @@ def controller_name
def initialize
@_request = nil
@_response = nil
@_response_body = nil
@_routes = nil
@_params = nil
super
end

Expand Down Expand Up @@ -240,11 +253,6 @@ def reset_session

class_attribute :middleware_stack, default: ActionController::MiddlewareStack.new

def self.inherited(base) # :nodoc:
base.middleware_stack = middleware_stack.dup
super
end

class << self
# Pushes the given Rack middleware and its arguments to the bottom of the
# middleware stack.
Expand Down
9 changes: 7 additions & 2 deletions actionview/lib/action_view/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,8 @@ def changed?(other) # :nodoc:
delegate :formats, :formats=, :locale, :locale=, :view_paths, :view_paths=, to: :lookup_context

def assign(new_assigns) # :nodoc:
@_assigns = new_assigns.each { |key, value| instance_variable_set("@#{key}", value) }
@_assigns = new_assigns
new_assigns.each { |key, value| instance_variable_set("@#{key}", value) }
Copy link
Member Author

Choose a reason for hiding this comment

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

I think ActionView::Base will inevitably be marked has having too many shapes, but at least we can keep a common root...

cc @tenderlove what do you think?

end

# :stopdoc:
Expand All @@ -232,9 +233,13 @@ def initialize(lookup_context, assigns, controller) # :nodoc:
@view_renderer = ActionView::Renderer.new @lookup_context
@current_template = nil

assign(assigns)
assign_controller(controller)
_prepare_context

super()

# Assigns must be called last to minimize the number of shapes
assign(assigns)
end

def _run(method, template, locals, buffer, add_to_stack: true, has_strict_locals: false, &block)
Expand Down
4 changes: 4 additions & 0 deletions actionview/lib/action_view/helpers/controller_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ def assign_controller(controller)
@_request = controller.request if controller.respond_to?(:request)
@_config = controller.config.inheritable_copy if controller.respond_to?(:config)
@_default_form_builder = controller.default_form_builder if controller.respond_to?(:default_form_builder)
else
@_request ||= nil
@_config ||= nil
@_default_form_builder ||= nil
end
end

Expand Down
6 changes: 5 additions & 1 deletion actionview/lib/action_view/test_case.rb
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,10 @@ def view
:@_result,
:@_routes,
:@controller,
:@_controller,
:@_request,
:@_config,
:@_default_form_builder,
:@_layouts,
:@_files,
:@_rendered_views,
Expand All @@ -243,7 +247,7 @@ def view
:@view_context_class,
:@view_flow,
:@_subscribers,
:@html_document
:@html_document,
]

def _user_defined_ivars
Expand Down
1 change: 1 addition & 0 deletions activemodel/lib/active_model/validations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,7 @@ def validate!(context = nil)
private
def init_internals
super
@errors = nil
@validation_context = nil
end

Expand Down
2 changes: 2 additions & 0 deletions activerecord/lib/active_record/attribute_methods/dirty.rb
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,8 @@ def init_internals
super
@mutations_before_last_save = nil
@mutations_from_database = nil
@_touch_attr_names = nil
@_skip_dirty_tracking = nil
end

def _touch_row(attribute_names, time)
Expand Down
12 changes: 5 additions & 7 deletions activerecord/lib/active_record/autosave_association.rb
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,6 @@ def self.valid_options

module ClassMethods # :nodoc:
private
def inherited(base)
super
base.class_eval do
@_already_called = nil
end
end

def define_non_cyclic_method(name, &block)
return if method_defined?(name, false)

Expand Down Expand Up @@ -280,6 +273,11 @@ def changed_for_autosave?
end

private
def init_internals
super
@_already_called = nil
end

# Returns the record for an association collection that should be validated
# or saved. If +autosave+ is +false+ only new records will be returned,
# unless the parent is/was a new record itself.
Expand Down
1 change: 1 addition & 0 deletions activerecord/lib/active_record/persistence.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1123,6 +1123,7 @@ def touch(*names, time: nil)
def init_internals
super
@_trigger_destroy_callback = @_trigger_update_callback = nil
@previously_new_record = false
end

def strict_loaded_associations
Expand Down
58 changes: 38 additions & 20 deletions activerecord/lib/active_record/reflection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,14 @@ def inherited(subclass)
# PolymorphicReflection
# RuntimeReflection
class AbstractReflection # :nodoc:
def initialize
@class_name = nil
@counter_cache_column = nil
@inverse_of = nil
@inverse_which_updates_counter_cache_defined = false
@inverse_which_updates_counter_cache = nil
end

def through_reflection?
false
end
Expand Down Expand Up @@ -260,10 +268,13 @@ def check_validity_of_inverse!
#
# Hence this method.
def inverse_which_updates_counter_cache
return @inverse_which_updates_counter_cache if defined?(@inverse_which_updates_counter_cache)
@inverse_which_updates_counter_cache = klass.reflect_on_all_associations(:belongs_to).find do |inverse|
inverse.counter_cache_column == counter_cache_column
unless @inverse_which_updates_counter_cache_defined
@inverse_which_updates_counter_cache = klass.reflect_on_all_associations(:belongs_to).find do |inverse|
inverse.counter_cache_column == counter_cache_column
end
@inverse_which_updates_counter_cache_defined = true
end
@inverse_which_updates_counter_cache
end
alias inverse_updates_counter_cache? inverse_which_updates_counter_cache

Expand Down Expand Up @@ -354,6 +365,7 @@ class MacroReflection < AbstractReflection
attr_reader :plural_name # :nodoc:

def initialize(name, scope, options, active_record)
super()
@name = name
@scope = scope
@options = options
Expand Down Expand Up @@ -457,6 +469,10 @@ def initialize(name, scope, options, active_record)
super
@type = -(options[:foreign_type]&.to_s || "#{options[:as]}_type") if options[:as]
@foreign_type = -(options[:foreign_type]&.to_s || "#{name}_type") if options[:polymorphic]
@join_table = nil
@foreign_key = nil
@association_foreign_key = nil
@association_primary_key = nil

ensure_option_not_given_as_class!(:class_name)
end
Expand Down Expand Up @@ -788,6 +804,7 @@ class ThroughReflection < AbstractReflection # :nodoc:
:active_record_primary_key, :join_foreign_key, to: :source_reflection

def initialize(delegate_reflection)
super()
@delegate_reflection = delegate_reflection
@klass = delegate_reflection.options[:anonymous_class]
@source_reflection_name = delegate_reflection.options[:source]
Expand Down Expand Up @@ -921,24 +938,23 @@ def source_reflection_names
end

def source_reflection_name # :nodoc:
return @source_reflection_name if @source_reflection_name

names = [name.to_s.singularize, name].collect(&:to_sym).uniq
names = names.find_all { |n|
through_reflection.klass._reflect_on_association(n)
}

if names.length > 1
raise AmbiguousSourceReflectionForThroughAssociation.new(
active_record.name,
macro,
name,
options,
source_reflection_names
)
@source_reflection_name ||= begin
names = [name.to_s.singularize, name].collect(&:to_sym).uniq
names = names.find_all { |n|
through_reflection.klass._reflect_on_association(n)
}

if names.length > 1
raise AmbiguousSourceReflectionForThroughAssociation.new(
active_record.name,
macro,
name,
options,
source_reflection_names
)
end
names.first
end

@source_reflection_name = names.first
end

def source_options
Expand Down Expand Up @@ -1042,6 +1058,7 @@ class PolymorphicReflection < AbstractReflection # :nodoc:
:name, :scope_for, to: :@reflection

def initialize(reflection, previous_reflection)
super()
@reflection = reflection
@previous_reflection = previous_reflection
end
Expand All @@ -1067,6 +1084,7 @@ class RuntimeReflection < AbstractReflection # :nodoc:
delegate :scope, :type, :constraints, :join_foreign_key, to: :@reflection

def initialize(reflection, association)
super()
@reflection = reflection
@association = association
end
Expand Down
5 changes: 5 additions & 0 deletions activerecord/lib/active_record/touch_later.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ def touch(*names, time: nil) # :nodoc:
end

private
def init_internals
super
@_defer_touch_attrs = nil
end

def surreptitiously_touch(attr_names)
attr_names.each do |attr_name|
_write_attribute(attr_name, @_touch_time)
Expand Down
2 changes: 2 additions & 0 deletions activerecord/lib/active_record/transactions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,9 @@ def trigger_transactional_callbacks? # :nodoc:

def init_internals
super
@_start_transaction_state = nil
@_committed_already_called = nil
@_new_record_before_last_commit = nil
end

# Save the new record state and id of a record so it can be restored later if a transaction fails.
Expand Down
2 changes: 2 additions & 0 deletions activesupport/lib/active_support/encrypted_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ def message
def initialize(config_path:, key_path:, env_key:, raise_if_missing_key:)
super content_path: config_path, key_path: key_path,
env_key: env_key, raise_if_missing_key: raise_if_missing_key
@config = nil
@options = nil
end

# Reads the file and returns the decrypted content. See EncryptedFile#read.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ def self.convert(number, options)

def initialize(number, options)
@number = number
@opts = options.symbolize_keys
@opts = options.symbolize_keys
@options = nil
end

def execute
Expand Down