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

fe: infer result type of redef via redefined feature #4023

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaellilltokiwa
Copy link
Member

@michaellilltokiwa michaellilltokiwa commented Oct 28, 2024

Does not work yet if result type contains a generic argument or this type.

@michaellilltokiwa michaellilltokiwa force-pushed the infer_result_type_via_redefined branch from 74701ea to 3b3216c Compare October 28, 2024 08:58
@michaellilltokiwa michaellilltokiwa marked this pull request as ready for review October 28, 2024 13:14
@michaellilltokiwa michaellilltokiwa requested a review from a team October 28, 2024 13:14
Copy link
Member

@fridis fridis left a comment

Choose a reason for hiding this comment

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

Nice.

I have some suggestions how to get this to work with type parameters and this type and some improvements.

Comment on lines +34 to +35
say c.b
say c.d
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
say c.b
say c.d
say c.b
say c.d
say (type_of c.b)
say (type_of c.d)

a is
b option String => "String"
d => option "String"
# NYI: UNDER DEVELOPEMENT result type with generic argument / this type
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to add examples that do not work yet to this comment.

{
AbstractType t = null;
var it = redefines().iterator();
while(t == null && it.hasNext())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
while(t == null && it.hasNext())
while (t == null && it.hasNext())

{
t = f.returnType().functionReturnType();
}
else if (!redefines().isEmpty())
Copy link
Member

Choose a reason for hiding this comment

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

This is always true

{
var redefined = it.next();
if (!(_returnType instanceof FunctionReturnType)
&& !redefines().isEmpty()
Copy link
Member

Choose a reason for hiding this comment

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

This is always true

@@ -2183,7 +2183,16 @@ else if (_impl.typeInferable())
{
if (CHECKS) check
(!state().atLeast(State.TYPES_INFERENCED));
result = _impl.inferredType(res, this, urgent);
var t = resultTypeFromRedefined(res);
// NYI: UNDER DEVELOPMENT: handle types that contain generic arguments / this types.
Copy link
Member

Choose a reason for hiding this comment

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

What is missing here is a call to handDownNonOpen(res, t, featureThatIsRedefined), then the generics and this types should be adjusted accordingly.

Comment on lines +2085 to +2097
/**
* Does this type contain a generic argument?
*
* @return
*/
public boolean containsGenericArgument()
{
return isGenericArgument()
|| !isGenericArgument() && (generics().stream().anyMatch(g -> g.containsGenericArgument()) ||
outer() != null && outer().containsGenericArgument());
}


Copy link
Member

Choose a reason for hiding this comment

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

Use dependsOnGenerics instead.

Comment on lines +2240 to +2243
&& redefined instanceof Feature f
&& f.returnType() instanceof FunctionReturnType)
{
t = f.returnType().functionReturnType();
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to use the code in the else branch in this case as well, i.e., resolveTypes and resultTypeIfPresent.

# -----------------------------------------------------------------------

infer_result_type_via_redefined =>

Copy link
Member

Choose a reason for hiding this comment

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

I am missing some test cases:

  • redefining two or more inherited features with equal types
  • redefining result types a and b as void
  • redefining a.f unit, b.f void, c.f String in
      a is
        f unit =>
      b is
        f => do
      c is
        f => "hi"
      d : a, b, c is
        redef f => panic "bla"
    
    should IMHO work.
  • negative tests

@michaellilltokiwa michaellilltokiwa marked this pull request as draft November 6, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants