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

Directive argument name and value ranges are incorrect for single-line directives #71

Closed
ethan-kusters opened this issue Aug 23, 2022 · 7 comments · Fixed by #79
Closed
Assignees
Labels
bug Something isn't working

Comments

@ethan-kusters
Copy link
Contributor

ethan-kusters commented Aug 23, 2022

Summary

Directive argument name and value ranges are incorrect for single-line directives that are written anywhere besides the first line of the document.

Details

I've reproduced this issue with the following test run on the current main branch:

func testDirectiveWithOneArgOnDifferentLines() throws {
    var source = """
        @Options(scope: page)
        """
    
    for line in 1...10 {
        let document = Document(parsing: source, options: .parseBlockDirectives)
        let directive = try XCTUnwrap(document.child(at: 0) as? BlockDirective)
        let arguments = directive.argumentText.parseNameValueArguments()
        let scopeArg = try XCTUnwrap(arguments["scope"])
        
        XCTAssertEqual("scope", scopeArg.name)
        XCTAssertEqual(
            SourceLocation(line: line, column: 10, source: nil)..<SourceLocation(line: line, column: 15, source: nil),
            scopeArg.nameRange,
            "Unexpected name range when authored at line '\(line)'"
        )
        
        XCTAssertEqual("page", scopeArg.value)
        XCTAssertEqual(
            SourceLocation(line: line, column: 17, source: nil)..<SourceLocation(line: line, column: 21, source: nil),
            scopeArg.valueRange,
            "Unexpected value range when authored at line '\(line)'"
        )
        
        source = "\n" + source
    }
}

which gives the following output:

XCTAssertEqual failed: ("Optional(Range(2:10..<2:15))") is not equal to ("Optional(Range(2:1..<2:6))") - Unexpected name range when authored at line '2'
XCTAssertEqual failed: ("Optional(Range(2:17..<2:21))") is not equal to ("Optional(Range(2:8..<2:12))") - Unexpected value range when authored at line '2'
XCTAssertEqual failed: ("Optional(Range(3:10..<3:15))") is not equal to ("Optional(Range(3:1..<3:6))") - Unexpected name range when authored at line '3'
XCTAssertEqual failed: ("Optional(Range(3:17..<3:21))") is not equal to ("Optional(Range(3:8..<3:12))") - Unexpected value range when authored at line '3'
XCTAssertEqual failed: ("Optional(Range(4:10..<4:15))") is not equal to ("Optional(Range(4:1..<4:6))") - Unexpected name range when authored at line '4'
XCTAssertEqual failed: ("Optional(Range(4:17..<4:21))") is not equal to ("Optional(Range(4:8..<4:12))") - Unexpected value range when authored at line '4'
XCTAssertEqual failed: ("Optional(Range(5:10..<5:15))") is not equal to ("Optional(Range(5:1..<5:6))") - Unexpected name range when authored at line '5'
XCTAssertEqual failed: ("Optional(Range(5:17..<5:21))") is not equal to ("Optional(Range(5:8..<5:12))") - Unexpected value range when authored at line '5'
XCTAssertEqual failed: ("Optional(Range(6:10..<6:15))") is not equal to ("Optional(Range(6:1..<6:6))") - Unexpected name range when authored at line '6'
XCTAssertEqual failed: ("Optional(Range(6:17..<6:21))") is not equal to ("Optional(Range(6:8..<6:12))") - Unexpected value range when authored at line '6'
XCTAssertEqual failed: ("Optional(Range(7:10..<7:15))") is not equal to ("Optional(Range(7:1..<7:6))") - Unexpected name range when authored at line '7'
XCTAssertEqual failed: ("Optional(Range(7:17..<7:21))") is not equal to ("Optional(Range(7:8..<7:12))") - Unexpected value range when authored at line '7'
XCTAssertEqual failed: ("Optional(Range(8:10..<8:15))") is not equal to ("Optional(Range(8:1..<8:6))") - Unexpected name range when authored at line '8'
XCTAssertEqual failed: ("Optional(Range(8:17..<8:21))") is not equal to ("Optional(Range(8:8..<8:12))") - Unexpected value range when authored at line '8'
XCTAssertEqual failed: ("Optional(Range(9:10..<9:15))") is not equal to ("Optional(Range(9:1..<9:6))") - Unexpected name range when authored at line '9'
XCTAssertEqual failed: ("Optional(Range(9:17..<9:21))") is not equal to ("Optional(Range(9:8..<9:12))") - Unexpected value range when authored at line '9'
XCTAssertEqual failed: ("Optional(Range(10:10..<10:15))") is not equal to ("Optional(Range(10:1..<10:6))") - Unexpected name range when authored at line '10'
XCTAssertEqual failed: ("Optional(Range(10:17..<10:21))") is not equal to ("Optional(Range(10:8..<10:12))") - Unexpected value range when authored at line '10'

showing that the name and value ranges are incorrect when the directive is authored beyond line 1.

Notes

This only reproduces for single-line directives and so seems related to #49.

@ethan-kusters ethan-kusters added the bug Something isn't working label Aug 23, 2022
@ethan-kusters
Copy link
Contributor Author

@Kyle-Ye this looks related to #49- any idea what might be going on here?

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Aug 23, 2022

let arguments = directive.argumentText.parseNameValueArguments()
// Not Compile since "arguements" is [DirectiveArguement]. Did you mean "arguments[0]"?
let scopeArg = try XCTUnwrap(arguments["scope"])

The code snippet seems not compile. Could you update it so that I can better reproduce the bug? @ethan-kusters

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Aug 23, 2022

@Kyle-Ye this looks related to #49- any idea what might be going on here?

Yes, I can reproduce it. I'm digging for more info and trying to solve it.

@ethan-kusters
Copy link
Contributor Author

The code snippet seems not compile. Could you update it so that I can better reproduce the bug? @ethan-kusters

@Kyle-Ye - looks like that test only works in BlockDirectiveParserTests, sorry about that! There's a fileprivate extension that adds the functionality:

fileprivate extension RandomAccessCollection where Element == DirectiveArgument {
    /// Look for an argument named `name` or log an XCTest failure.
    subscript<S: StringProtocol>(_ name: S, file: StaticString = #filePath, line: UInt = #line) -> DirectiveArgument? {
        guard let found = self.first(where: {
            $0.name == name
        }) else {
            XCTFail("Expected argument named \(name) but it was not found", file: file, line: line)
            return nil
        }
        return found
    }
}

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Aug 24, 2022

This a new bug needs to be fixed. Even I revert the change in #50, the test case you provide still fails.

A more simplified version to reproduce:

func testDirectiveWithOneArgOnDifferentLines() throws {
    let source = """

    @Options(scope: page)
    """

    let line = 2
    let document = Document(parsing: source, options: .parseBlockDirectives)
    let directive = try XCTUnwrap(document.child(at: 0) as? BlockDirective)
    let arguments = directive.argumentText.parseNameValueArguments()
    let scopeArg = try XCTUnwrap(arguments["scope"])

    XCTAssertEqual("scope", scopeArg.name)
    XCTAssertEqual(
        SourceLocation(line: line, column: 10, source: nil) ..< SourceLocation(line: line, column: 15, source: nil),
        scopeArg.nameRange,
        "Unexpected name range when authored at line '\(line)'"
    )

    XCTAssertEqual("page", scopeArg.value)
    XCTAssertEqual(
        SourceLocation(line: line, column: 17, source: nil) ..< SourceLocation(line: line, column: 21, source: nil),
        scopeArg.valueRange,
        "Unexpected value range when authored at line '\(line)'"
    )
}

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Aug 24, 2022

The block directive parser seems to parse properly. And you can verify it with the following code. (By testing on directive.argumentText.segments[0].range)

let source = """

@Options(scope: page)
"""

let line = 2
let document = Document(parsing: source, options: .parseBlockDirectives)
let directive = try XCTUnwrap(document.child(at: 0) as? BlockDirective)
XCTAssertEqual(
    directive.argumentText.segments[0].range,
    SourceLocation(line: line, column: 10, source: nil) ..< SourceLocation(line: line, column: 21, source: nil)
)

The root cause of the bug may be caused by the implementation of DirectiveArgumentText.parseNameValueArguments

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Sep 25, 2022

I have added a PR to fix it. Could you help review it? @ethan-kusters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants