Separate top-level semantic phase for overload reordering

[HertzDevil]

This is a more complete proposal of an idea that has been mentioned in several PRs.

Currently, whenever the top-level visitor encounters a new overload of a method, it reorders the overloads immediately, and in doing so it resolves the type restrictions in the def at that point of visitation; types and type relations that are declared later would not affect the order of the overload being added. This leads to problems such as #4897, #7579, #10518, and #11678.

The compiler should defer overload reordering to a distinct phase after top-level declarations are processed. What it means is:

• The top-level visitor should append a method and leave it as is, even for redefinitions.
• Somewhere in the top-level phase (say before abstract def checking), all overload sets are readded one by one, redefinitions are eliminated, and previous_defs are registered.
  • If Use more robust ordering between def overloads #10711 is available, a total order is imposed on all defs that aren't redefinitions, so we can simplify this step to a stable in-place sort followed by redefinition removals.
  • An easier way yet might be simply isolating Crystal::TopLevelVisitor#visit(node : Def) into a new visitor. This makes TypeNode#methods completely empty in top-level macros though.
• Any undefined constants at this point cannot be defined later during compilation, so we can optionally turn them into hard errors. Other forms of method validation may also occur here (such as Free variables not used in method's signature #7737).
• method_missing happens during main analysis, so it continues to trigger overload reordering every time a corresponding def is generated.
• Every new def in the interpreter still triggers overload reordering (sometimes you can do it and other times it says you cannot declare def dynamically, I don't know why yet).

This would be a breaking change because top-level macros cannot observe the intermediate overload ordering: (contrast with TypeNode#instance_vars)

class Foo
  def foo
  end

  {% Foo.methods.size %} # => 1

  def foo
    # redefinition is removed before main analysis
    {% Foo.methods.size %} # => 1
  end

  # redefinition will no longer be removed during top-level analysis
  {% Foo.methods.size %} # => 2
end

Foo.new.foo

An alternative solution would be #10723, but it requires changes to user code and probably a lot more effort to implement that feature.

[asterite]

Another breaking change is that, I think, method_added is called right away, but with this change they will all be stacked and invoked at "the end".

That said, I think we should do it, even if it's a breaking change. I don't think method_added or inspecting a type's methods at the top level is very useful.

[HertzDevil]

I believe method_added can still be expanded right away, the only differences being that:

• Like other top-level macros, method_added will see @type.methods as they are declared, without being reordered.
• If the method_added macro adds new defs, these also aren't immediately reordered.

[straight-shoota]

This sounds like a good idea.

Maybe it could be worth considering elimination of obvious redefinitions. def foo unconditionally redefines def foo, because the signatures are lexically identical. It could be a cause of confusion though, and as mentioned it's probably not that useful.

[Blacksmoke16]

Maybe it could be worth considering elimination of obvious redefinitions.

Ref: #10071

[HertzDevil]

def foo unconditionally redefines def foo, because the signatures are lexically identical

It is, in fact, not that obvious because defs that yield are not required to have a & parameter yet (#8764).

[straight-shoota]

It's confusing for TypeNode#methods to have a different order depending on where it's called (top level macro or inside a macro def).

What's even the purpose of making overload ordering observable here? I would consider this an unintended spillover effect of the implementation. I have actually never noticed that. It's not documented. And it's probably not very useful.
If that information is necessary for something, it should be exposed in a more explicit way.

But the order of TypeNode#methods should be consistent. Order of definition would probably be best. That means it cannot rely on ModuleType#@defs and we'd need a separate container to list them.

I don't think this should be handled in #11840. It works well as a follow-up.

However, a relevant aspect is what happens about redefinitions. I still think it would be nice if they can be eliminated right away. If requiring a & in the method signature to indicate a yielding method enables that, it should be worth it.
Unless there are any other problems for recognition of redefs?

[HertzDevil]

If the defs do not follow overload ordering it would become more difficult for me to diagnose the ordering-related issues right here 😂

A purely syntactic approach to detecting redefinitions "right away", i.e. in the top-level phase, would definitely miss redefinitions that require resolving Path nodes (e.g. between a type and its alias). IMO it would be more consistent if we remove all redefinitions or none of them, anything in-between should probably be avoided.