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

Always allow assignment to tagged:"any" #8

Closed
4 tasks done
mraak opened this issue Feb 12, 2019 · 18 comments
Closed
4 tasks done

Always allow assignment to tagged:"any" #8

mraak opened this issue Feb 12, 2019 · 18 comments
Assignees

Comments

@mraak
Copy link
Collaborator

mraak commented Feb 12, 2019

  • Always allow assignment to tagged:"any" but don't overwrite the type
  • allow call-expression arguments if the corresponding parameter type is any
  • if a function signature's return is set to any, return statements need to allow any return values
  • apply isAssignemntAllowed in other relevant places: #6 Number subtypes #28 (review)
@mraak mraak self-assigned this Feb 12, 2019
@getify
Copy link
Owner

getify commented Feb 13, 2019

Example:

var a = any``;
a = "hello";  // OK
a = 1; // also OK because `a` is still type `any`

var b = a + 2;   // error: mixed operand types: 'any' and 'number'

@mraak
Copy link
Collaborator Author

mraak commented Feb 14, 2019

Note (to self):

function typesMatch(type1,type2) {
 var type1ID = getTypeID(type1);
 var type2ID = getTypeID(type2);
 return (
  type1ID == "any" ||         // <==============
  type1ID != "unknown" &&
  type2ID != "unknown" &&
  type1ID === type2ID
};
}

@getify
Copy link
Owner

getify commented Feb 14, 2019

I don't think this check can go here, since typesMatch(..) is used in lots of places that don't relate to assignment, and this exception only applies to assignment. I think it needs to go into handleAssignmentExpressionType(..).

@mraak
Copy link
Collaborator Author

mraak commented Feb 18, 2019

	if (!typesMatch(sourceType,targetType)) {
						if (targetType.inferred == "undef"){
							delete targetType.inferred;
							Object.assign(targetType,sourceType);

							// NOTE: temporary debugging output
							if (isTaggedType(sourceType)) {
								addOutputMessage({
									id: MSG.INFO_REIMPLY_UNDEF_TAGGED,
									text: `Re-implying ${targetNode.name} with tagged-type '${sourceTypeID}'`,
									node: targetNode,
								});
							}
							else {
								addOutputMessage({
									id: MSG.INFO_REIMPLY_UNDEF_INFERRED,
									text: `Re-implying ${targetNode.name} to inferred-type '${sourceTypeID}'`,
									node: targetNode,
								});
							}
						}	else if(targetTypeID != "any") {  // <===========
							reportUnexpectedType(
								MSG.ERR_ASSIGNMENT_TYPE,
								"Assignment type mismatch",
								sourceType,
								targetType,
								exprNode
							);
						}
					}

Something like this maybe....

@getify
Copy link
Owner

getify commented Feb 18, 2019

Yeah I think that might work for the assignment. But we'll also need to add some other handling as well. For example:

  • allow call-expression arguments if the corresponding parameter type is any
  • if a function signature's return is set to any, return statements need to allow any return values

@mraak
Copy link
Collaborator Author

mraak commented Feb 18, 2019

Shall these two cases rather go into tickets for arguments/return handling, or is it better doing it here?

@getify
Copy link
Owner

getify commented Feb 18, 2019

Yeah I want to handle all cases with any in this ticket... there may be a few others we haven't fully thought through yet.

mraak added a commit that referenced this issue Feb 18, 2019
@mraak
Copy link
Collaborator Author

mraak commented Feb 20, 2019

allow call-expression arguments if the corresponding parameter type is any

Is this test case good enough?

function foo(a = string){}
function bar(a = any){}

foo(1) // error: argument type missmatch
bar(1) // pass

if a function signature's return is set to any, return statements need to allow any return values

Need test case for this one.

@getify
Copy link
Owner

getify commented Feb 20, 2019

That's good. Also:

function foo(x) { return x; }
function bar(y) { return y; }

var a = foo(any`3`);
var b = bar(4);

foo(b);  // no error
bar(a);  // argument type error

a = bar(5);  // ok
b = foo("oops");  // assignment type error (b)

@mraak
Copy link
Collaborator Author

mraak commented Feb 20, 2019

It reports this, so I think we might be good here.

(pass 1) ------------------------
Implying foo as inferred-type 'func'
Function 'foo' signature: {"type":"func","params":[{"inferred":"unknown"}],"hasRestParam":false,"return":{"default":true,"inferred":"undef"}}
Implying bar as inferred-type 'func'
Function 'bar' signature: {"type":"func","params":[{"inferred":"unknown"}],"hasRestParam":false,"return":{"default":true,"inferred":"undef"}}
Implying parameter x from argument, as tagged-type 'any'
Implying parameter y from argument, as inferred-type 'number'
(pass 2) ------------------------
Function 'foo' signature: {"type":"func","params":[{"tagged":"any"}],"hasRestParam":false,"return":{"tagged":"any"}}
Function 'bar' signature: {"type":"func","params":[{"inferred":"number"}],"hasRestParam":false,"return":{"inferred":"number"}}
Implying a as tagged-type 'any'
Implying b as inferred-type 'number'
Argument type mismatch: expected type 'number', but found type 'undef'
Assignment type mismatch: expected type 'number', but found type 'any

@getify
Copy link
Owner

getify commented Feb 20, 2019

There's a bug here somehow:

Argument type mismatch: expected type 'number', but found type 'undef'

That should say "expected type 'number' but found type 'any'".

The return value of foo(..) is any on the second pass, and thus a gets implied as any, so then when passing a as the argument, its type should still be any.

@mraak
Copy link
Collaborator Author

mraak commented Feb 21, 2019

I see. Is this to be resolved in this ticket or is it a part of multipass? Because without this, I can already send a PR.

@getify
Copy link
Owner

getify commented Feb 21, 2019

I don't think it's related to multi-pass. But it might be.

If you don't think you can find/fix the bug, let's just file it as a separate issue, then you can close/merge the PR and branch, and I can take a look at why it's happening.

mraak added a commit that referenced this issue Feb 21, 2019
mraak added a commit that referenced this issue Feb 21, 2019
@mraak
Copy link
Collaborator Author

mraak commented Feb 21, 2019

Check it out, I created a PR. I'll also have a look. I added line numbers to the output, I think it helps a lot.

Interesting that in the output you can see that it is implying it as any, but then sees it as undef later.

@getify
Copy link
Owner

getify commented Feb 21, 2019

Fixed. See PR #26.


nice job on the line numbers, btw. That was on my list of things to get to (that's why I added the whole node value to addOutputMessage(..).

@mraak
Copy link
Collaborator Author

mraak commented Feb 21, 2019

Merged and I see the test case now works as it should.

@getify
Copy link
Owner

getify commented Feb 21, 2019

BTW, make sure you look at #27 that I just filed. Nothing to do (it's not a bug), but it's a nuanced quirk that we should both keep in mind.

mraak added a commit that referenced this issue Feb 22, 2019
* #8 Allowing assignement to any, handling any as argument. Added line and column display to messages.

* #8 indentation

* checker: fixing bug where 'any' and 'undef' were confused (#26)
mraak added a commit that referenced this issue Feb 23, 2019
* #6 Number subtypes

* #6 Added 'any' check to isAssignmentAllowed

* #8 any (#29)

* #8 Allowing assignement to any, handling any as argument. Added line and column display to messages.

* #8 indentation

* checker: fixing bug where 'any' and 'undef' were confused (#26)

* #6 merged any check and line number reporting.

* #6 code review fixes, allowed assignment lookup improvement.
@mraak
Copy link
Collaborator Author

mraak commented Feb 25, 2019

I believe this one could be closed now.

@getify getify closed this as completed Feb 25, 2019
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

No branches or pull requests

2 participants