-
Notifications
You must be signed in to change notification settings - Fork 33
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
Composite literal inference #704
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are making good progress. That's great!
You should keep an eye on isSuper
and TypeOf
. These functions are accumulating too much complexity. At some point you should consider a refactoring session. I know it's easier said than done. For example you could start with extracting common predicates, splitting big functions, reduce indentation, explore table-driven approaches.
I further recommend "Refactoring" by Martin Fowler and similar online resources. They provide many patters to improve code.
if n, ok := Predefined[n.String()]; ok { | ||
return n | ||
} | ||
if n.String() == "infinity" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you check if this condition is ever true?
At least it should never be true, because the infinity
token is supposed to a syntax.ValueLiteral
(like true
, inconc
, ...) and not syntax.Ident
.
If it es ever true it must be a bug in the parser, which needs fixing.
@@ -458,6 +479,177 @@ func TypeOf(n syntax.Expr) Type { | |||
case syntax.INC, syntax.DEC: | |||
return Predefined["integer"] | |||
} | |||
case *syntax.ParenExpr: | |||
// different length? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: syntax.ParenExpr
represents either a (template) list, e.g. (1,2,3)
or it presents an expression such as 1 * (2+3)
.
For now its okay to use the length of the list as indicator. But there is some unresolved ambiguity, we need to resolve: (1)
could be be a template list or an expression. Which means we properly must infer the proper type from its context.
return TypeOf(n.List[0]) | ||
} | ||
case *syntax.IndexExpr: | ||
if n.X == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please comments explaining the conditions. Short examples are okay.
return nil | ||
} | ||
|
||
func mostSpecificCommonTypeOfArray(types ...Type) Type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write a documentation comment for this function.
if len(fields) == 0 { | ||
return &ListType{Kind: RecordOf} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this condition to the top of the case-block to improve the flow when reading.
Also, you can remove one indirection. Write if len(n.List) == 0
instead of if len(fields) = 0
.
return result | ||
} | ||
|
||
func mostSpecificCommonTypeOf2(type1 Type, type2 Type) Type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has mostSpecificCommonTypeOf2
a different behaviour than mostSpecificCommonTypeOfArray
?
return Predefined["any"] | ||
} | ||
|
||
func isSuper(type1 Type, type2 Type) Type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before merging you should rename and document this function. isSuper
suggests a boolean, but it returns a Type
.
No description provided.