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

The presence of toString on error makes it far too easy to ignore errors #350

Closed
sameerajayasoma opened this issue Oct 2, 2019 · 4 comments
Assignees
Labels
Area/Lang Relates to the Ballerina language specification design/dislike Do not like something about the design design/usability Design does not work well for some tasks

Comments

@sameerajayasoma
Copy link
Contributor

sameerajayasoma commented Oct 2, 2019

if (orderPayload is json) {
             string orderId = orderPayload.Order.ID.toString();
}

The type of the expression orderPayload.Order.ID is json | error. Now because the programer has used toString(), the error goes unnoticed. I think this can be a potential problem. I.e., programmer errors.

@sameerajayasoma sameerajayasoma added Area/Lang Relates to the Ballerina language specification design/dislike Do not like something about the design labels Oct 2, 2019
@jclark jclark added the design/usability Design does not work well for some tasks label Oct 2, 2019
@jclark
Copy link
Collaborator

jclark commented Oct 2, 2019

This really undermines the error checking.

6.20 in the spec says "If the static type of expression is a subtype of some basic type with identifier B, and the module lang.B contains a function method-name then M is B. The identifier for a basic type is the reserved identifier used in type descriptors for subtypes of that basic type, as listed in the Lang library section." But in this case the expression is not a subtype of a basic type. So the spec actually disallows this. But what the spec says is too strict, because it would disallow using it on a value of anydata.

So we need to fix up 6.20 to have some rules on how unions are handled, and those rules need to disallow unions with error and non-error.

@jclark jclark self-assigned this Oct 2, 2019
@jclark jclark added this to the 2019R4 milestone Oct 2, 2019
@jclark jclark modified the milestones: 2019R4, 2020R1 Dec 21, 2019
@jclark jclark modified the milestones: 2020R1, 2020R2 Feb 24, 2020
@jclark jclark modified the milestones: 2020R3, 2020R2 Apr 24, 2020
@jclark
Copy link
Collaborator

jclark commented Apr 25, 2020

Let's do this together with #499.

@jclark jclark modified the milestones: 2020R2, 2020R3 Jun 17, 2020
@sameerajayasoma sameerajayasoma added the status/discuss Needs discussion outside github comments label Nov 17, 2020
@jclark
Copy link
Collaborator

jclark commented Nov 19, 2020

The only functions this is a problem for are toString and toBalString in lang.value, which both take arguments of type any|error. Another approach to solving this is to split each of these into two functions

  • one in lang.value, which takes an argument of type any
  • one in lang.error, which takes an argument of type error

This allows us to have the natural rule in 6.20 for lang.value functions: you can have a union type that is a subtype of the argument type specified in the function signature.

jclark added a commit that referenced this issue Nov 20, 2020
@jclark
Copy link
Collaborator

jclark commented Nov 20, 2020

My comment in #350 (comment) was mistaken. There's another bullet in the spec for method-call-expr that covers this:

Otherwise, M is value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area/Lang Relates to the Ballerina language specification design/dislike Do not like something about the design design/usability Design does not work well for some tasks
Projects
None yet
Development

No branches or pull requests

2 participants