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

Nested function types support #294

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

as3boyan
Copy link
Contributor

@as3boyan as3boyan commented Jun 4, 2015

Nested function types support
#293

@as3boyan
Copy link
Contributor Author

as3boyan commented Jun 6, 2015

If I recall correctly I was getting tests passed when I tested it locally, on travis it shows lots of failed test cases with some wierd info

@EBatTiVo
Copy link
Contributor

EBatTiVo commented Jun 8, 2015

on travis it shows lots of failed test cases

The problem is the debugger sources aren't compiling correctly on Travis using this branch or master -- the tests never even run. However, they work perfectly on my machine. I've even tried a git clone into a new directory and using the make test workflow. This may be a Travis-only problem.

I'm not sure whether this warning has anything to do with it:

[javac2] warning: [options] bootstrap class path not set in conjunction with -source 1.6

@yanhick Any ideas on why this would fail on Travis, but not on a local workstation?

@yanhick
Copy link
Contributor

yanhick commented Jun 8, 2015

You probably have a mismatch between your local config and Travis's. Maybe
Java version ?

The description of the Travis build env is here:
http://docs.travis-ci.com/user/ci-environment/

For the Java warning, I found this:
http://stackoverflow.com/questions/7816423/warning-options-bootstrap-class-path-not-set-in-conjunction-with-source-1-5

Hope this helps !

2015-06-08 14:35 GMT-07:00 Eric B [email protected]:

on travis it shows lots of failed test cases

The problem is the debugger sources aren't compiling correctly on Travis
using this branch or master -- the tests never even run. However, they work
perfectly on my machine. I've even tried a git clone into a new directory
and using the make test workflow. This may be a Travis-only problem.

I'm not sure whether this warning has anything to do with it:

[javac2] warning: [options] bootstrap class path not set in conjunction with -source 1.6

@yanhick https://github.com/yanhick Any ideas on why this would fail on
Travis, but not on a local workstation?


Reply to this email directly or view it on GitHub
#294 (comment).

@EBatTiVo
Copy link
Contributor

EBatTiVo commented Jun 8, 2015

On my local machine, I always use Sun JDK 1.6. Travis doesn't have that.

I'm thinking that this may, indeed, be a change to JDK 1.7. Maybe a security update that Travis has deployed? At any rate, to build with Java 1.7 on Travis we're going to have to play with the compiler settings.

@yanhick
Copy link
Contributor

yanhick commented Jun 8, 2015

I find this doc about Travis build environnement update:
http://docs.travis-ci.com/user/build-environment-updates/

There doesn't seem to be any change since April.

Yes you could update the build to target Java 1.7 or alternately you could
install the JDK 1.6 on Travis by using Ubuntu repositories.

2015-06-08 14:57 GMT-07:00 Eric B [email protected]:

On my local machine, I always use Sun JDK 1.6. Travis doesn't have that.

I'm thinking that this may, indeed, be a change to JDK 1.7. Maybe a
security update that Travis has deployed? At any rate, to build with Java
1.7 on Travis we're going to have to play with the compiler settings.


Reply to this email directly or view it on GitHub
#294 (comment).

@EBatTiVo
Copy link
Contributor

EBatTiVo commented Jun 8, 2015

AFAIK, Ubuntu is not licensed to distribute any Sun/Oracle JDK through its repositories. Oracle has discontinued JDK1.6 except for some large corporate licenses; it's no longer available on their web sites.

@yanhick
Copy link
Contributor

yanhick commented Jun 8, 2015

You're right, I forgot about that, I always install openjdk and don't
bother with the Oracle website.

2015-06-08 15:30 GMT-07:00 Eric B [email protected]:

AFAIK, Ubuntu is not licensed to distribute any Sun/Oracle JDK through its
repositories. Oracle has discontinued JDK1.6 except for some large
corporate licenses; it's no longer available on their web sites.


Reply to this email directly or view it on GitHub
#294 (comment).

}

return true;
return HaxeComponentType.typeOf(element) != HaxeComponentType.FIELD;
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to leave the old code in place for these types of changes. I write them as separate returns when I need to break in the debugger on a certain return value, because you can't set a breakpoint on the return after the expression has been evaluated. The other pattern that I use is

ret = expr; 
return ret;

My thinking for leaving them is that if I needed the returns separate for debugging in the past, then I'll probably need it again, and it doesn't make the code any harder to understand. It's just a style thing, though.

@EBatTiVo
Copy link
Contributor

Yannick has fixed the Travis build for the moment by not regenerating the Java code for the debugger protocol. He's filed issue #305.

Can you please merge master down into this branch and re-push. Once we get a clean compile, this is approved for merge.

@EBatTiVo
Copy link
Contributor

On second thought. Wait to merge until after we pull Carlos' change, which should be today if we don't find any more issues.

@EBatTiVo
Copy link
Contributor

@as3boyan Carlos' change has been pulled. You may proceed at your leisure.

@EBatTiVo
Copy link
Contributor

EBatTiVo commented Feb 4, 2016

@boyan This branch was approved for merge a while back. Do you remember why it wasn't merged? Is it still valid?

The build error was because 14.1.1 was no longer available on the Jetbrains site. The correct course of action is probably to merge master into this branch locally and then push it back up so that we can get a current build and test run.

@as3boyan
Copy link
Contributor Author

as3boyan commented Feb 4, 2016

I don't remember, but I remember that we have some issue with nested function (now I can't even imagine how nested functions look)
Needs to be tested and unit tests

Conflicts:
	src/14.1/com/intellij/plugins/haxe/runner/debugger/HaxeDebugRunner.java
@EBatTiVo
Copy link
Contributor

Note that the new tests I just added fail. This is because the golden masters haven't been properly created yet. And that is because I think that the parsing result is incorrect (and probably was incorrect before this change).

This line:

typedef MyFnPtr = String->Int->Void;

creates this PSI:

  TYPEDEF_DECLARATION
    PsiJavaToken:typedef('typedef')
    COMPONENT_NAME
      IDENTIFIER
        PsiJavaToken:ID('MyFnPtr')
    PsiJavaToken:=('=')
    FUNCTION_TYPE
      FUNCTION_TYPE
        TYPE_OR_ANONYMOUS
          TYPE
            REFERENCE_EXPRESSION
              IDENTIFIER
                PsiJavaToken:ID('String')
        PsiJavaToken:->('->')
        TYPE_OR_ANONYMOUS
          TYPE
            REFERENCE_EXPRESSION
              IDENTIFIER
                PsiJavaToken:ID('Int')
      PsiJavaToken:->('->')
      TYPE_OR_ANONYMOUS
        TYPE
          REFERENCE_EXPRESSION
            IDENTIFIER
              PsiJavaToken:ID('Void')
    PsiJavaToken:;(';')

Note how the FUNCTION_TYPE element contains a nested FUNCTION_TYPE element, instead of a series of TYPE_OR_ANONYMOUS elements. When processing the code looking for a function type, we won't see a proper function type, rather it will be interpreted as:

typedef MyFnPtr = (String->Int)->Void;

which is the wrong answer.

The problem lies in these three lines in the BNF:

private functionTypeWrapper ::= functionTypeOrWrapper functionType*
private functionTypeOrWrapper ::= typeOrAnonymous | '(' functionTypeWrapper ')'
left functionType ::= '->' '?'? functionTypeOrWrapper

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

Successfully merging this pull request may close these issues.

3 participants