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

As operator #3201

Closed
wants to merge 12 commits into from
Closed

As operator #3201

wants to merge 12 commits into from

Conversation

RyanCavanaugh
Copy link
Member

Implements as operator as suggested in #296.

@@ -669,6 +670,12 @@ module ts {
right: Expression;
}

export interface AsExpression extends Expression {
left: Expression;
asToken: Node;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need this token?

Copy link
Member

Choose a reason for hiding this comment

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

Don't think we need to store the as token.

@DanielRosenwasser
Copy link
Member

Can we have a few tests for templates in tests/cases/.../es6/templates/?

var x = `${123 + 456 as number}`;
var y = `leading ${123 + 456 as number}`;
var y = `${123 + 456 as number} trailing`;
var x = `Hello ${123} World` as string;
var y = `Hello` as string;
var z = 1 + `${1} end of string` as string;
declare function tag(...x: any[]): any;

var x = tag `Hello ${123} World` as string;
var y = tag `Hello` as string;

@yuit
Copy link
Contributor

yuit commented May 19, 2015

Should we add formating rule for as?
For example:

var x = 42   as string;

will become

var x = 42 as string;

@yuit
Copy link
Contributor

yuit commented May 19, 2015

Could we add some tests:

var a = 20;
var b = a as string;
var as = "hello";
var as1 = as as string;

@tinganho
Copy link
Contributor

I'm just wondering if we can support inferred type assertions and if it is a good idea?

Instead of:

(foo as Foo).(bar as Bar).text

We could just use:

foo.bar.text

And it will infer the type by looking at the last property.

@DanielRosenwasser
Copy link
Member

Should we add formating rule for as?

If by formatting rule, you mean a restriction, yes (good catch!), but not related to the example you just gave (nowhere else in the language do we differentiate whitespace on the same line). Consider the following:

class Foo { }
declare function as(...args: any[]);

// Example 1
var x = 10
as `Hello world`

// Example 2
var y = 20
as(Foo);

Example 1 is not as much of a problem; you can't use a template string as a type.

Example 2 suffers from potentially the same problem as #2995. as would ordinarily be a function call with a constructor function, but here it is a type assertion on 20 to Foo.

@DanielRosenwasser
Copy link
Member

@yuit Ah, by formatting rule, you meant in the LS - still, glad we caught this.

@RyanCavanaugh RyanCavanaugh mentioned this pull request May 19, 2015
@yuit
Copy link
Contributor

yuit commented May 19, 2015

@DanielRosenwasser yes, I mean from LS side similar to:

function       foo() {}

become

function foo() {}

Though you example will be a good one to add as well.

if (token === SyntaxKind.AsKeyword) {
parseTokenNode();
leftOperand = makeAsExpression(leftOperand, parseType());
} else {
Copy link
Member

Choose a reason for hiding this comment

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

else on the next line.

@RyanCavanaugh
Copy link
Member Author

Any other feedback?

@@ -2818,7 +2821,16 @@ module ts {
break;
}

leftOperand = makeBinaryExpression(leftOperand, parseTokenNode(), parseBinaryExpressionOrHigher(newPrecedence));
if (token === SyntaxKind.AsKeyword) {
if (canParseSemicolon()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just add a comment for this case.

// This should be parsed as an initialized variable, followed by a function call to 'as' with the argument 'Bar'
if (canParseSemicolon()) {
break;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

else on the next line

@DanielRosenwasser
Copy link
Member

You're currently not contextually typing the left-hand side of the as operator. So if you take

var x = (v => v) as (x: number) => number

v gets typed as any right now. This means that

var x = (v => v) as (x: number) => string

currently typechecks without a problem even though

var x = <(x: number) => string>(v => v)

gives an error.

@DanielRosenwasser
Copy link
Member

Can we also have the following tests to demonstrate left-associativity of as?

asOperatorAssociativity01.ts

var x = 10 as number as any as string // should be okay

asOperatorAssociativity02.ts

var y = 10 as string as number; // should error

@RyanCavanaugh
Copy link
Member Author

Anything else?

@DanielRosenwasser
Copy link
Member

Are we leaving the services layer to a second pass? I can think of at least:

  • Formatting tests
  • Does as get occurrence highlighting to point out type assertion locations? (kind of useful; it's the point of static_cast etc. in C++).
  • Completion list tests for arrow functions:
class C<T> {
    constructor() {
        // C, T, and U should show up below.
        let f = <U>(x: any) => x as /**/ 
type A = any;
namespace n {
    type B = any;
    // A, B, T, and n should show up below.
    let f = <T>(x: any) => x as /**/ 
  • Completion list tests to ensure that you never have a new identifier location after an as.

@RyanCavanaugh RyanCavanaugh mentioned this pull request May 27, 2015
@DanielRosenwasser
Copy link
Member

I am working on a change such that keywords like interface must be followed by an identifier on the same line to be considered the start of an interface declaration.

But as is only a type assertion if it follows an identifier on the same line. So there's an ambiguity.

It seems like the appropriate thing to do is to treat it as an declaration named as.

You'll have to account for this with some tests like:

interface as { }
interface
as
{ }
interface
as({ })
namespace as {}
declare as {}
type as = number;

// as (Bar)
// This should be parsed as an initialized variable, followed
// by a function call to 'as' with the argument 'Bar'
if (canParseSemicolon()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just check scanner.hasPrecedingLineBreak instead? I don't really like canParseSemicolon that much.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 9, 2015

@RyanCavanaugh is this ready to go in?

@RyanCavanaugh
Copy link
Member Author

I'm merging this up with the JSX work, which should have a PR in a day or so

@mhegazy
Copy link
Contributor

mhegazy commented Jun 19, 2015

closing in favor of #3564

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants