Skip to content

Not allow function overloads which only diff in parameters' ranks#8140

Merged
ke-yu merged 6 commits intoDynamoDS:masterfrom
ke-yu:rank-overload
Sep 7, 2017
Merged

Not allow function overloads which only diff in parameters' ranks#8140
ke-yu merged 6 commits intoDynamoDS:masterfrom
ke-yu:rank-overload

Conversation

@ke-yu
Copy link
Contributor

@ke-yu ke-yu commented Aug 30, 2017

Purpose

This PR disables function overloads which only diff in parameters' ranks. That is, the following function overloads are not allowed because parameter xss have the same type int (though with different ranks)

def foo(xs: int[]) {
    ...
}
def foo(xs: int[][]) {
    ...
}

But the following overloads are still valid:

def foo(x: int) {
    ...
}
def foo(x: string) {
    ...
}
def foo(x: int, y: int) {
   ...
}

The main purpose of this change is to reduce the complexity of method resolution for overloaded functions, especially in replication, and to make the program be more predicable from user's point of view. For example, in the following code, it is not so obvious which foo() will be invoked.

def foo(xs: int[], ys: int[][]) {
    ...
}
def foo(xs: int[][], ys: int[]) {
    ...
}
// both foo()s are valid candidates
r1 = foo({{1, 2}, {3, 4}}, {{5, 6}, {7, 8}});    

 // should we replicate on first foo() for the first argument?
r2 = foo({{1, 2}, 3}, {{4, 5}, {6, 7}});        

This PR removes overloads for the following builtins (Note even these builtins have been obsolete. They are replaced by functions in List namespace)

  • Equals(var[]..[], var[]..[]), Equals(var, var)
  • IndexOf(var[]..[], var[]..[]), IndexOf(var[]..[], var)
  • Insert(var[]..[], var[]..[], int), Insert(var[]..[], var, int)
  • Print(var[]..[]), Print(var)
  • GetType(var[]..[]), GetType(var)

If there are overloads, the rule is the first one wins. There could be other more sophisticated rules, e.g., the function with higher rank parameter wins. One reason of using this simple rule is in delta execution environment, we don't want a newly added function disables its overload and makes the graph stop working.

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@aparajit-pratap

FYIs

@kronz @Racel

@ke-yu ke-yu requested a review from aparajit-pratap August 30, 2017 07:03
@ke-yu ke-yu changed the title [WIP] Not allow function overloads which only diff in parameters' ranks Not allow function overloads which only diff in parameters' ranks Aug 31, 2017
@aparajit-pratap
Copy link
Contributor

aparajit-pratap commented Aug 31, 2017

@ke-yu I don't see nodes for these built-ins:

IndexOf(var[]..[], var[]..[])
Insert(var[]..[], var, int)
Print(var[]..[]), Print(var)
GetType(var[]..[]), GetType(var)

}
}

return f;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do you check for the rank of the parameters here in order to return the function with the highest ranks?

Copy link
Contributor Author

@ke-yu ke-yu Sep 1, 2017

Choose a reason for hiding this comment

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

First come first serve. Suppose in delta execution environment we define a function and a properly working graph, now we define a overload which has higher rank of its parameter, then the original function will be pushed out of procedure table and the whole graph just stops working.

Instead of being smart, the rule is simple - take the first one.

@ke-yu
Copy link
Contributor Author

ke-yu commented Sep 4, 2017

@aparajit-pratap yep these builtins are obsolete, not available on UI.

@ke-yu
Copy link
Contributor Author

ke-yu commented Sep 7, 2017

@kronz @Racel any comments?

@Racel
Copy link
Contributor

Racel commented Sep 7, 2017

@ke-yu - I am completely fine with this approach. Function overloads that differ in parameter ranks were always confusing anyway, and IIRC this process started earlier with nodes like ExportToSat, etc. In short, LGTM

@ke-yu ke-yu merged commit 3034d8a into DynamoDS:master Sep 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants