Issue 915 - Get which @:overload classfield was called - haxe

[issuesbot]

[Google Issue #915 : https://code.google.com/p/haxe/issues/detail?id=915] by waneck, at 10/06/2012, 19:27:55 In order to implement overloaded calls correctly for Java/C# we need a way to know which classfield was actually called

[issuesbot]

[comment from waneck, published at 11/06/2012, 22:35:12]

[issuesbot]

[comment from simon.kr...@simn.de, published at 14/06/2012, 21:16:44]

[issuesbot]

[comment from simon.kr...@simn.de, published at 14/06/2012, 21:56:16] Requires typed AST changes?

[issuesbot]

[comment from ncanna...@gmail.com, published at 16/06/2012, 16:56:11] It does indeed.

[issuesbot]

[comment from simon.kr...@simn.de, published at 16/06/2012, 17:12:24] Should we for starters add a call_infos type to TCall? That way it's not such a big deal to modify it later.

[issuesbot]

[comment from ncanna...@gmail.com, published at 16/06/2012, 19:14:42] What would call_infos contain ?

Also, I'm not sure exactly what we need here : the tclass_field or something more ? For instance we might want to know which class the tclass_field belong to, but then you need the full inheritance tree + parameters in order to be able to apply type parameters.

[issuesbot]

[comment from simon.kr...@simn.de, published at 16/06/2012, 19:20:31] I'm not sure what we need either, which is why I think a new type makes sense so we can decide/try that later without having to modify the generators (again).

[issuesbot]

[comment from waneck, published at 16/06/2012, 19:25:33] I like the idea of having a call_infos type. But right now I think that the only info we really need is the tclass_field called, which is the only info I can't retrieve. The originated tclass would be handy to have, but it's easy to get just by looking at the underlying TField type.

Now, one thing that would be great would be to have a cf_id : int field, just like we have on tvar (v_id), so we could identify classfields that have the same path (path * string) :)

[issuesbot]

[comment from waneck, published at 16/06/2012, 19:36:10] anyway, I think it's a great idea to have a call_infos type so we could later extend the info it contains, if needed.

[issuesbot]

[comment from ncanna...@gmail.com, published at 19/06/2012, 20:23:15] I think that maybe the best change would be to do the following change

TField of expr * field_access

type field_access = | FStatic of tclass_field | FMember of tclass_field | FDynamic of string | FEnum of tenum * tenum_field | ... ?

And remove TEnumField

[issuesbot]

[comment from waneck, published at 19/06/2012, 20:26:30] Really like it!! :)

[issuesbot]

[comment from waneck, published at 22/06/2012, 19:46:13] Just thought of a nastier problem: We need also to get the same @ :overload info for TNew!

[issuesbot]

[comment from simon.kr...@simn.de, published at 14/07/2012, 09:47:04] TNew is a special case because there's not even a promise that a class field is involved at all from the tAST point of view. It just so happens that we currently resolve it to a class constructor field all the time, which begs the question if we even need TNew to begin with.

We could instead have a TCall(TField(...),args) where the field_access is FConstructor of tclass * tparams * tclass_field.

Yet another way would be to allow tparams for TTypeExpr, so a ctor would (syntactically speaking) become YourClass<S,T>.new(args). This would then follow normal class field semantics, including overload behavior.

[issuesbot]

[comment from ncanna...@gmail.com, published at 14/07/2012, 10:52:45] In Haxe type parameters are only for instances and member methods so TTypeExpr+tparams does not make much sense.

We could maybe remove TNew but I think it's better to have a bit more high level constructs. Adding the class_field to it seems ok since we only deal with classes so far.

Most of the targets will be able to safely ignore the field_access, or we can add a small helper field_access_name

[issuesbot]

[comment from ncanna...@gmail.com, published at 14/07/2012, 10:54:10]

[issuesbot]

[comment from ncanna...@gmail.com, published at 04/09/2012, 16:36:22] I gave it two tries today.

The first one involved removing TEnumField and TClosure directly, but then it was tedious to do all the changes in a conservative manner, so I rolled it back.

The second one just changed TField with the following definition :

| TField of texpr * tfield_access

and tfield_access = | FInstance of tclass * tclass_field | FStatic of tclass * tclass_field | FAnon of tclass_field | FDynamic of string

The tclass here being the class in which the actual field was found

However I couldn't finished it, I did most of the code including all generators (except genCommon/genCS/genJava), the only remaining part being typer.ml, which is the most important one since it builds the TField, but there's only around 25 of them so should be doable soon.

I tried doing a bit of local refactoring when possible but usually it will just use previous implementation and add a field_name call to extract the name string from the tfield_access

Two additional passes will be necessary after we have this done :

a) remove TEnumField and add FEnum to tfield_access b) remove TClosure and add FClosure to tfield_access : in that case I think we also need to know if it's a static closure or an instance one

That's actually quite a lot of work as a total - more than abstract types actually - I might then prioritize other features before finishing this one.

[issuesbot]

[comment from waneck, published at 04/09/2012, 20:02:41] Nicolas, I really like the changes. If you finish the core (typer.ml) ones, I compromise to convert the remaining changes to use the new TField signature.

Thanks

[issuesbot]

[comment from simon.kr...@simn.de, published at 05/10/2012, 12:51:45] What about access_kind? AKField would seem to be the same as the new AKExpr(TField) and both AKInline and AKMacro are just TFields where the field has the inline/macro flag. Maybe the change could be used to reduce complexity a little here?

[issuesbot]

[comment from ncanna...@gmail.com, published at 05/10/2012, 22:00:23] Yes, after we have the tfield_access infos, that could be a good refactor as well But we need to complete first the field_kind.patch

[issuesbot]

[comment from simon.kr...@simn.de, published at 05/10/2012, 22:45:40] My thinking was to use field_name as you suggested in the matches, and FDynamic for the builds for now. This should be roughly equivalent to the current TField.

We can then go ahead and replace the FDynamics with proper field_access types during refactoring. I think this ends up being cleaner and easier than trying to get the access right at the moment where we often don't even know if we're talking static access or not.

See attached for a unit test-compiling patch (Java and CS targets being disabled).

[issuesbot]

[comment from simon.kr...@simn.de, published at 06/10/2012, 13:29:26] Priority up because small changes are likely to cause conflicts with this big patch.

[issuesbot]

[comment from ncanna...@gmail.com, published at 23/12/2012, 15:07:46] I have committed the first part of the TField changes, will open an issue with the rest of things to do

[issuesbot]

[comment from waneck, published at 22/02/2013, 23:45:48]