This repository has been archived by the owner. It is now read-only.

[Flow] Function predicate declaration #103

Merged
merged 6 commits into from Feb 10, 2017

Conversation

Projects
None yet
5 participants
@panagosg7
Copy link
Contributor

panagosg7 commented Aug 31, 2016

The accepted syntax for function declarations is extended to allow
the specification of a function predicate (inferred or explicit). This
should be placed right after the function's return type (if existing,
otherwise the colon that would designate the start of the return
type). So the return type syntax becomes:

FunctionReturnType :=
  Type
  Predicate
  Type Predicate

Predicate :=
  %checks
  %checks ( ConditionalExpression )

These changes are with regards to this commit in the Flow repo.

[Flow] Function predicate declaration
The accepted syntax for function declarations is extended to allow
the following predicate declaration:

  FunctionReturnType :=
    Type
    Predicate
    Type Predicate

  Predicate :=
    %checks
    %checks ( ConditionalExpression )
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 31, 2016

Current coverage is 97.29% (diff: 100%)

Merging #103 into master will increase coverage by 0.02%

@@             master       #103   diff @@
==========================================
  Files            21         21          
  Lines          4001       4035    +34   
  Methods         485        491     +6   
  Messages          0          0          
  Branches       1179       1180     +1   
==========================================
+ Hits           3892       3926    +34   
  Misses           49         49          
  Partials         60         60          

Powered by Codecov. Last update 1150c0d...e42d60c

this.expectContextual("checks");
// Force '%' and 'checks' to be adjacent
if (moduloLoc.line !== checksLoc.line || moduloLoc.column !== checksLoc.column - 1) {
this.raise(moduloPos, "Unexpected token");

This comment has been minimized.

@danez

danez Sep 2, 2016

Member

This line is not covered. Do you think we can have two tests that check if there are parse errors in these cases?

This comment has been minimized.

@danez

danez Sep 2, 2016

Member

You could also use this.unexpected(moduloPos); here. It's a little bit shorter but otherwise isn't really important if not changed.

@danez

This comment has been minimized.

Copy link
Member

danez commented Sep 2, 2016

Thank you for your first PR to babylon.

In order to get this working throughout the complete chain of babel, all the new node types (I think it is only DeclaredPredicate and InferredPredicate) need to be added to babel-types and babel-generator
Could you do that and create a PR for babel?

I would wait with the merge till we have a PR in babel and the new flow version is released.

Great work!

@panagosg7

This comment has been minimized.

Copy link
Contributor Author

panagosg7 commented Sep 2, 2016

Thanks @danez! I'll address your comments.

@panagosg7 panagosg7 closed this Sep 2, 2016

@panagosg7 panagosg7 reopened this Sep 2, 2016

@panagosg7

This comment has been minimized.

Copy link
Contributor Author

panagosg7 commented Sep 4, 2016

@danez: This can still wait until Flow's upcoming release.


pp.flowParseTypeAndPredicateInitialiser = function () {
let oldInType = this.state.inType;
this.state.inType = true;

This comment has been minimized.

@danez

danez Oct 7, 2016

Member

this.state.inType seems to be only necessary for this.flowParseType(), so maybe we could just do this two lines in the else branch? Then the reset only has to happen there either.

This comment has been minimized.

@panagosg7

panagosg7 Jan 17, 2017

Author Contributor

It looks like this.state.inType needs to be true for this.expect(tt.colon) to succeed (I tried to move it further down, but other tests failed). So, oldInType needs to be recorded in the beginning in order to be restored before this.flowParsePredicate() is called (note that flowParsePredicate possibly parses expressions).

Bottom line is I wasn't able to perform the optimization you suggested.

this.expectContextual("checks");
// Force '%' and 'checks' to be adjacent
if (moduloLoc.line !== checksLoc.line || moduloLoc.column !== checksLoc.column - 1) {
this.unexpected(moduloPos);

This comment has been minimized.

@danez

danez Oct 7, 2016

Member

Can you add a nice error message here? Maybe something like "Spaces between ´%´ and ´checks´ are not allowed here."

The second argument to this.unexpected can be a custom message.

this.unexpected(moduloPos);
}
if (this.match(tt.parenL)) {
this.next();

This comment has been minimized.

@danez

danez Oct 7, 2016

Member

Instead of

if (this.match(tt.parenL)) {
    this.next();

you can do

if (this.eat(tt.parenL)) {

Which is doing the exact same thing but is shorter. :)

@panagosg7

This comment has been minimized.

Copy link
Contributor Author

panagosg7 commented Oct 8, 2016

Thanks for the pointers, @danez!

I will get to these, but I'm still waiting for support for function predicates to be fully merged in Flow master.

@danez danez added the PR: WIP label Nov 11, 2016

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented Jan 15, 2017

I know flow supports this now, is this waiting on more changes, or has it just been forgotten?

@panagosg7 panagosg7 force-pushed the panagosg7:master branch from ef8a254 to d5782a9 Jan 17, 2017

@panagosg7

This comment has been minimized.

Copy link
Contributor Author

panagosg7 commented Jan 17, 2017

Sorry for taking so long to get back to this.

AFAIK, there is still some function predicate code that hasn't been merged to Flow's master. Most of these changed involve the type checker, but there is also some parser work under way. For example I believe the notation of predicate refinement will be updated to something more elegant. Also, there has not been an official announcement yet.

However, you are right that some support for function predicates (including %checks) is already in mainline and recognized by the native Flow parser, so I guess there should be support here as well.

I recently rebased my original PR, addressed @danez 's comments (including the ones in the respective babel PR) and added one more example.

@leebyron

This comment has been minimized.

Copy link

leebyron commented Feb 1, 2017

Any progress on this? I'd love to see this land to unblock early use

@danez

danez approved these changes Feb 10, 2017

@danez danez merged commit e049ec3 into babel:master Feb 10, 2017

3 checks passed

codecov/patch 100% of diff hit (target 97.27%)
Details
codecov/project 97.29% (+0.02%) compared to 1150c0d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danez danez removed the PR: WIP label Feb 10, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.