Skip to content

The future of SyntaxWalker #1728

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

Closed
ashwinr opened this issue Jan 19, 2015 · 12 comments
Closed

The future of SyntaxWalker #1728

ashwinr opened this issue Jan 19, 2015 · 12 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@ashwinr
Copy link

ashwinr commented Jan 19, 2015

TSLint relies heavily on walkers and visitors, so I'm figuring out the next steps since these are no longer exposed via the language service. Since I still see visitors in the 1.4 release internally, what is the future of this logic? Will it be removed as mentioned in #254? Thanks!

@danquirk danquirk added the Question An issue which isn't directly actionable in code label Jan 19, 2015
@danquirk
Copy link
Member

Tagging @mhegazy @CyrusNajmabadi @vladima

@ashwinr
Copy link
Author

ashwinr commented Jan 19, 2015

thanks @danquirk ! would love an answer soon, as I have to update the linter for the 1.4 compiler. As an example of how a linter rule is implemented, please see https://github.com/palantir/tslint/blob/master/src/rules/classNameRule.ts

@vladima
Copy link
Contributor

vladima commented Jan 19, 2015

Visitors\walkers should be removed from the source code soon so don't take dependencies on them. Current recommended solution is to use forEachChild for tree traversal - in your case code for your rule should look something like this

// I have not idea what this type looks like
interface RuleFailure { }
function checkNameCaseForClassesAndInterfaces(s: ts.SourceFile) {
    var failures: RuleFailure[] = [];
    walk(s);
    return failures;

    function walk(n: ts.Node) {
        switch (n.kind) {
            case ts.SyntaxKind.ClassDeclaration:
            case ts.SyntaxKind.InterfaceDeclaration:
                if (!isPascalCase((<ts.InterfaceDeclaration>n).name)) {
                    failures.push(addFailure(<ts.InterfaceDeclaration>n))
                }
                break;
            case ts.SyntaxKind.SourceFile:
            case ts.SyntaxKind.ModuleDeclaration:
                // dive deeper into modules\source file
                ts.forEachChild(n, walk);
            default: // other kinds of nodes cannot contains classes and interfaces - skip them
        }
    }
}

function isPascalCase(name: ts.Identifier): boolean {
    throw "NYI"
}

function addFailure(n: ts.InterfaceDeclaration): RuleFailure {
    throw "NYI"
}

Please let me know if there are any questions regarding the API.

Also since release 1.4 we've published compiler API for public usage, you can find more information in wiki Using the Compiler API

@gscshoyru
Copy link

Hey guys -- we're looking more into moving over our code. It seems that tokens are no longer visited by the new walker pattern, correct? With this change, how should we go about implementing rules such as https://github.com/palantir/tslint/blob/master/src/rules/oneLineRule.ts , where we want to make sure that opening braces are on the same line as the statement preceding them, and we want to make sure if and else statements are followed by whitespace. So far as I can tell, there's no good way to do this besides getting the fullText of the statement, and doing regex to check these things, which is not ideal. Additionally, can you explain what you've done with the trivia in more detail? The API documentation is somewhat confusing in this regard, and we have a decent number of rules that care about whitespace (https://github.com/palantir/tslint/blob/master/src/rules/noConsecutiveBlankLinesRule.ts) and ones that care about comments (https://github.com/palantir/tslint/blob/master/src/rules/commentFormatRule.ts), and I wouldn't mind some clarification for how we should deal with those. Thanks!

@vladima
Copy link
Contributor

vladima commented Jan 20, 2015

Trees in the compiler don't store tokens (except identifiers, regex\numeric\string literals and template strings). However it is possible to query the node for the list of its children by calling getChildren - result list will contain both nodes and tokens (represented as one type Node).
SourceFile.getLineAndCharacterFromPosition can be used to obtain line\char for a given position. These values are 1-based. file.getLineAndCharacterFromPosition(node.getStart(file)) will return you the starting line \ character of the node so to check if tokens are separated with more than one line you can call this function for both tokens and then compare '.line' values. Also as an option for eliminating consecutive blank lines you can use createScaner with skipTrivia=false - instantiate the scanner and then sweep the file in one pass.

@jasonscharf
Copy link

Wow, great docs around the compiler API so far. Thanks guys!

@mhegazy
Copy link
Contributor

mhegazy commented Jan 20, 2015

@jasonscharf, please let us know if there are topics missing or not well covered in the docs. this is still work-in-progress, and we would love to get your feedback.

@gscshoyru
Copy link

Hey guys, another question -- how exactly does ts.getLeadingCommentRanges work? The documentation suggests it should take both a node and position -- but instead in the typings file it takes a piece of text (a string), and a position. What piece of text should you pass it?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jan 22, 2015

how exactly does ts.getLeadingCommentRanges work?

I've recently written some documentation on it: https://github.com/Microsoft/TypeScript/wiki/Architectural-Overview#trivia

To reiterate, this function is in the spirit of Roslyn's view of leading trivia.

So what this basically means is that when giving the full start of a node, it will read until the next newline and then start extracting all comment ranges until the actual start.

/ * h e l l o * / _ _ _ _ _ [CR] [NL] _ _ _ _ / / b y e [CR] [NL] _ _ / * h i * / _ _ _ _ f u n c t i o n 
↑                                     ↑       ↑                       ↑                   ↑
full start                       look for     first comment           second comment      node start
                           leading comments 
                            starting here

What piece of text should you pass it?

The file's source text.

@gscshoyru
Copy link

Yeah, that's the documentation I was looking at. I would suggest re-wording it, and/or including an example or two (or that picture you just made there, that picture is great), since it's not clear what to pass into it from the documentation -- especially since the trivia was actually attached to the tokens themselves before the architecture change, so my expectation was that getting the trivia would be node-specific, rather than position-specific. It makes a lot more sense now that you've demonstrated how it works, though.

And thanks for the help.

@DanielRosenwasser
Copy link
Member

Addressed, and you're very welcome.

@Tartarwyxx
Copy link

@jasonscharf

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

8 participants