[typer] fix forwarded static extension priority

[vonagam]

Closes #9680 if it is agreed upon.

In the test couldn't mark extension classes as private, gave them mangled names instead.

[Simn]

I agree with fixing this, but the fix looks a bit awkward like that. I'll have to check if an approach like this is really necessary.

[vonagam]

No prob. using_field does some mutations like add_dependency, mark_import_position and remove_constant_flag and i treated it as not cheap one, so had to make sure that it isn't called more than once successfully and no more than once for an abstract itself.

If using_field is to be considered cheap (when it does not find anything, so full run), then i can remove half of the change lines in fields.ml, which are connected to using_field_called ensuring "no more than once for an abstract itself".

[vonagam]

Updated PR to apply the same logic to @:resolve.

Also removed commented out "this" thing. It was added in 17ee33c. I guess at the time this inside an abstract wasn't typed as it is now.

[vonagam]

Rewrote by hand and now it is green...

[vonagam]

Rebased. Any comments on new implementation?

[Simn]

This whole ref stuff looks pretty magical. I'd have to check in detail, but I'm worried that this really complicates matters.

Maybe we should approach this from the other direction and write down what we actually want. I think we need to invert some control here by having functions such as these:

find_field_on_type t
find_type_static_extension_for t
find_module_static_extension_for t
find_resolve_on t

If these are atomic, we can then compose type_field exactly how we want. I imagine it would be along the lines of:

| TAbstract (a,pl) as t ->
	let rec walk_abstract t f =
		try
			f t
		with Not_found -> match t with
			| TAbstract(a,tl) when not (Meta.has Meta.CoreType a.a_meta) ->
				walk_abstract (Abstract.get_underlying_type ~return_first:true a tl) f
			| _ ->
				raise Not_found
	in

	begin try
		walk_abstract t find_field_on_type with Not_found -> try
		walk_abstract t find_type_static_extension_for with Not_found -> try
		walk_abstract t find_module_static_extension_for with Not_found -> try
		walk_abstract t find_resolve_on with Not_found ->
		no_field()
	end

This checks all "stages" of an abstract for each kind of lookup, and then tries the next kind.

What do you think?

[Simn]

Of course the recursion in walk_abstract should also check if the field is actually forwarded.

[vonagam]

I like it. Went for current design because wanted to keep source code for other parts of type_field in tact, minimising disruption.

So do you plan to do the split by yourself in rework branch (so this PR will be closed) or should i do it here?

[Simn]

I'll be working on unary operators on the next rework branch, so you can have this one!

[vonagam]

Here is rewrite.

[Simn]

At a first glance it looks good! I'll review it in detail tomorrow.

These {e with etype = ... } parts made me remember #6639. In general, I don't know if we really want to recurse with a different texpr, or if it's better to just work with Type.t everywhere and leave e unchanged.

However, it might be a good idea to just aim for parity with this PR, and then address questions like that as a follow-up.

Two other related issues I found: #6870 and #5097.

[vonagam]

I encountered those things myself while initially implementing "support @:using on typedefs" (which is, by the way, already rebased on this one). That's why there is ?t in main type_field_by functions. Instead of {e with etype = ...} it is possible to just pass new type to ~t and keep old e (as typedef does).

About linked issues and this PR:
Closes #6870 as this is what started the PR (fixing order of checking static extensions).
Closes #5097 as this is exactly what i asked in slack about (forward to dynamic vs extension/resolve).
Even though it is easy to do #6639 now, it is a breaking change, so definitely should not be done in this PR.

[Simn]

Note for the future: Check why we're catching both of these. That seems weird.

[Simn]

Ok I've checked it. I don't like this ~t/get_t stuff very much, so I think we should lose that in a subsequent change, but other than that the overall structure is very good.

I'd like @RealyUniqueName to take a look at this too.

[vonagam]

Any specific reason? How otherwise would you achieve what you mentioned before "type expression, but (optionally) with different type than it is"?

Regarding @:forward and losing abstract information - don't know which type should be reported, but it is interesting that TType is expected to remain, where TAbstract with forward to be lost. This fact doesn't seem right, TType shouldn't be more opaque than TAbstract.

[Simn]

What I mean is that we can just work with the e that is the argument to type_field, and all these inner functions can use that while recursing with a different/more specific t. We can then check if we want to insert TCast nodes in places where we use e with a t that is different than its etype. However, there's still the problem in MSet mode because we can't do (cast e : t) = value.

[RealyUniqueName]

Another issue related to losing abstract type info upon forwarding: #6639

[RealyUniqueName]

I agree with @Simn regarding ~t/get_t. It's better to explicitly pass e.etype instead of omitting an argument if no distinct t is expected.

[RealyUniqueName]

It's a TAnon case. I guess this check is out of place.

[vonagam]

TAnon can have AbstractStatics status, this is for this case.
Here is a test that checks that this is in place.

[RealyUniqueName]

Then it probably should be moved to match !(a.a_status) below.

[vonagam]

Another issue related to losing abstract type info upon forwarding: #6639

This one is the first mentioned issue (don't counting one in PR description).

I agree with @Simn regarding ~t/get_t. It's better to explicitly pass e.etype instead of omitting an argument if no distinct t is expected.

As i understand his remarks were about replacing, in the future, passing down e with passing down t instead. Not about usage of the optional argument per itself. And right now get_t is not followed if ~t is passed, which allows not to follow 4 times initial e.etype and also less verbose in one-two places. But ok, will update that.

[RealyUniqueName]

In the code you either follow type before passing it to ~t or follow e.etype inside of get_t. Which means following happens anyway, but in different places (inside or outside of a function).
It's a minor concern, but since it's known at a call site I think it would be more consistent and easier to understand if following happens outside only. This way it would be possible to call the function without following should the need arise.
For example

type_field_by_type t e
(* instead of *)
type_field_by_type ~t e

or

type_field_by_type (follow e.etype) e
(* instead of *)
type_field_by_type e

That would allow to drop get_t

[vonagam]

Already pushed removal of optionality from t.