Type hinting #116

krisavi · 2019-11-18T13:55:08Z

What kind of change does this PR introduce?
I made quite few changes like type hinting // #type: vesselTarget over the weekend.
PR adds typehinting with syntax // #type: typestring and // #returns: typestring
It also takes the parameters for functions to generate function signature.
There is some residue code from testing out:
Noticed that I had to do 2 passes over the whole code, because kos allows to have functions defined after calling them, but the code currently would give warning about signature not found.
The code is not the prettiest and not the cleanest, but could be useful.
What is the current behavior? (You can also link to an open issue here)
No possibility of code hinting
What is the new behavior (if this is a feature change)?
Possibility of code hinting
Problems/Limitation:
Currently the limitation is that 1 parameter per line, if there are more, then it still takes first typestring and applies it to first parameter in the line.
There are some issues still. on function you can have // #type: typestring and // #returns: typestring. Both work the same. Needs way to differentiate between 2 of them.
Type-hinting at the moment only lets you to enter only one type. Not possible to make it something like exist() command has (string or path)
Probably should change the syntax to work more like // #parametername: typestring for parameters.
Other information:
Some changes in suffixes and such that came out as problems when testing code. Probably another PR.

krisavi · 2019-11-18T14:03:35Z

I guess the automated test kind of got stuck.

krisavi · 2019-11-18T14:10:33Z

Seems I caused some memory leak issue in tests which I have to fix in PR.

jonnyboyC

Overall awesome PR! This is something I've been trying to get around to for a while so thanks for getting in there and getting it in there. Did some testing and it all appeared to work as expected which is really exciting! I've some comments most of them are very minor, most minor cleanup things. Feel free to ignore most of them.

If you could I would write a test or two in typeChecker.spec.ts specifically for your new type hints. Maybe something as simple as

function example { #returns: string
  parameter x // #type: string

  return x.
}

Then just check that example has one parameter of string that returns string. Again big thanks this is the first PR against the repo that wasn't from me. So any feedback for me on how the code is organize would be appreciated as well!

jonnyboyC · 2019-11-19T00:39:19Z

server/src/models/token.ts

@@ -57,6 +60,10 @@ export class Token implements TokenBase {
    };
  }

+  public get getRef(): Token | undefined {


Probably don't need this getter here since this.ref already is available.

jonnyboyC · 2019-11-19T00:54:23Z

server/src/parser/models/declare.ts

@@ -103,13 +103,15 @@ export class Var extends Decl {
  public readonly toIs: Token;
  public readonly value: IExpr;
  public readonly scope: Scope;
+  public readonly typeHint?: ITypeHint;


Since we're applying typeHint to each declaration type I think we should move it up to the Decl super class and just pass typeHint into super in each case.

I started with only parameters.
Later added other ones.

constructor is using builder for subclasses, but not for superclass. Seems to be few issues just moving to to superclass.

jonnyboyC · 2019-11-19T00:55:40Z

server/src/parser/parser.ts

@@ -75,6 +79,9 @@ export class Parser {
      this.logger.info(
        `Parsing started for ${file} with ${this.tokens.length} tokens.`,
      );
+      this.logger.info(
+        `Parsing started for ${file} with ${this.types.length} tokens.`,


maybe we could have something like Parsing started for ${file} with ${this.types.length} type hints.

jonnyboyC · 2019-11-19T01:32:33Z

server/src/scanner/scanner.ts

@@ -114,6 +120,17 @@ export class Scanner {
      kind: ScanKind.Token,
    };

+    this.typeResult = {


Might be good to make a helper function now that we have three of these now. Maybe something like

private createTokenResult(tokenType: TokenType, scanKind: ScanKind): TypeResult { // ... }

I am not actually even sure why do we need that placeholder declaration here.

I just made a copy without going much into details why it has to be in here. They all seem like placeholders which we should not need to create.

I think your right, I'm not sure why it's there either as it seems we assign it then just return it in the various methods. My guess was there was some idea I half implemented then abandoned. I would say feel free to remove these and update the token generation methods. If not I can do some cleanup in a different PR

jonnyboyC · 2019-11-19T01:37:29Z

server/src/scanner/scanner.ts

+          case ScanKind.Type:
+            // Were not able to reference earlier to lookback
+            // Gets first identifier on the line where type is defined
+            result.result.ref = tokens.filter(token => token.type === TokenType.identifier && token.start.line == result.result.start.line)[0];


I think this is fine but we might want to think about some like a line buffer so we're searching through fewer tokens. I did pull down your branch and it doesn't appear issue.

It seems as long as the array we are searching from (token array) does not grow too big, then it should not be a problem
https://github.com/dg92/Performance-Analysis-JS

On my biggest script I have token array of around 6000 elements.

Maybe we just find instead of filter with index 0. I think its more clear that we're just looking for one element plus in the repo you linked to shows better performance so that seems like a win win.

Yeap, changed it to find.

jonnyboyC · 2019-11-19T02:04:51Z

server/src/typeChecker/typeChecker.ts

@@ -182,7 +197,11 @@ export class TypeChecker
    const { tracker } = decl.identifier;

    if (this.isBasicTracker(tracker)) {
-      tracker.declareType(result.type.assignmentType());
+      if (decl.typeHint && TypeHint.get(decl.typeHint.toString())) {
+          tracker.declareType(TypeHint.get(decl.typeHint.toString()) || result.type.assignmentType());


I think a future PR we could try to report an warning if the typehint isn't assignable to the inferred type. Maybe something like this

local x is "some string" // #type: scalar // ^--- could probably indicate that scalar isn't assignable to string local y is ship:parts[0] // #type: engine // ^-- engine is a part so this should be ok

The common case will be structure since that when the type checker gives up which anything should be assignable to it

jonnyboyC · 2019-11-19T02:11:37Z

server/src/typeChecker/typeChecker.ts

+    }
+
+
+    // debug code:


Would probably be good to clean this up

jonnyboyC · 2019-11-19T02:13:27Z

server/test/typeChecking.spec.ts

@@ -736,7 +736,7 @@ describe('Statements', () => {
    const [first, second] = params;

    expect(first).toBe(structureType);
-    expect(second).toBe(createUnion(true, structureType, noneType));
+    expect(second).toBe(createUnion(true, integerType, noneType));


jonnyboyC · 2019-11-19T02:21:04Z

server/src/parser/parser.ts

@@ -242,9 +249,10 @@ export class Parser {
  }

  // parse function declaration
-  private declareFunction(scope?: Decl.Scope): INodeResult<Decl.Func> {
+  private declareFunction(scope?: Decl.Scope, typeHint?: Maybe<ITypeHint>): INodeResult<Decl.Func> {


It doesn't appear anything is passed for typeHint here or in the other declare functions below. I think it would be good to remove them for now if this was temporary code.

jonnyboyC · 2019-11-19T02:24:54Z

server/src/models/token.ts

@@ -32,6 +34,7 @@ export class Token implements TokenBase {
    this.start = start;


Since we only set this in the scanner I think we should just update the constructor and make ref readonly

constructor( type: TokenType, lexeme: string, literal: any, start: Position, end: Position, uri: string, ref?: Token) { // ... }

krisavi · 2019-11-19T09:18:31Z

I was thinking of doing couple of tests yes, It needs some improvements, I found out some more issues with it.
As a first step adding tests is way to go yes.

One more thing I remembered now that is missing is dynamic list generation, Parser needs improvements. I think I have defined 2-3 lists, Type hints are currently defined in one file and it is unknown what are the types they can use, Maybe autocomplete could be used to give some hints on what kind of type definitions there are.

Will look into writing some tests and improving code.

krisavi · 2019-11-19T12:24:59Z

It seems the parser config used in tests is not defining types. Have to figure out why it is ignoring all the hints.

jonnyboyC · 2019-11-19T15:09:28Z

To your first comment, I think an autocomplete would definitely be a good thing to have. I think it would probably make sense to add it in a separate PR.

For type hinting depending on which spec file your in you may need to add typeInitializer() before the tests. This is located in "/src/typeChecker/initialize". Essentially this has to be run because some of the types have circular references to each other. This is especially true with the more primitive types like structure, boolean etc.

krisavi · 2019-11-19T20:59:50Z

Did some magic with automatic detection of return.

krisavi · 2019-11-20T17:32:21Z

Did struggle with testing. Not sure if i can make it work. During the test, it has not done the visitParameter it seems, so it has the type declared as structure and not picking up the updated one. That is the reason why it crashes for me. Function on the other hand had parameter types defined already, which is odd. So it needs some more figuring out how I can force the token types to be defined by the time the test takes place. It seems if I run it 2 times before test checks, everything is in order. Wanted to check the order it gets parameters-variables and such declared. Hoping I will find a way to make sure it is in order that will also not make it lag with bigger scripts.

For longer script I put my whole launch, orbit, rendezvous and deorbit and recovery with last part as suicide burn, while doing science collection and transmission as one big script. Basically took all the libraries and stitched it as one file to put language server to the test.

jonnyboyC · 2019-11-21T00:04:43Z

Just as a point of reference I typically gauge speed against kerboscripts/parse_valid/ap.ks. it's about 2kloc. I believe it's the script for that sort of famous kOS quad copter.

Feel free to push code to your PR if you need another set of eyes on it. I can take a stab at writing some tests if you need some help. Definitely very interested in getting this merged!

Type hinting

44eb9ed

Fix the test of automatic function signature

7d2c689

jonnyboyC reviewed Nov 19, 2019

View reviewed changes

jonnyboyC added the ready-for-review To indicate this PR is ready to be reviewed label Nov 19, 2019

jonnyboyC assigned krisavi Nov 19, 2019

		@@ -32,6 +34,7 @@ export class Token implements TokenBase {
		this.start = start;

Type hinting #116

Are you sure you want to change the base?

Type hinting #116

Uh oh!

Conversation

krisavi commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krisavi commented Nov 18, 2019

Uh oh!

krisavi commented Nov 18, 2019

Uh oh!

jonnyboyC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krisavi commented Nov 19, 2019

Uh oh!

krisavi commented Nov 19, 2019

Uh oh!

jonnyboyC commented Nov 19, 2019

Uh oh!

krisavi commented Nov 19, 2019

Uh oh!

krisavi commented Nov 20, 2019

Uh oh!

jonnyboyC commented Nov 21, 2019

Uh oh!

Uh oh!

krisavi commented Nov 18, 2019 •

edited

Loading