Specify disambiguation rules in YAML

[smola]

Specify disambiguation rules in a YAML file. See rationale in #3746.

Description

All disambiguation rules are moved from heuristics.rb into heuristics.yml.

Checklist:

• I am adding new or changing current functionality
  • I have added or updated the tests for the new or changed functionality.

[smola]

This is now ready for review, although it could use some more testing and cleanup of heuristics.rb.

[vmg]

@smola: Looks like the heuristics got a bit messed up during the translation, as two language identification tests are failing. Can you look at them?

[smola]

Fixed negative matches.
Also rebased this on top of #4189 to improve verification of the change. Ideally #4189 would be merged first.

[vmg]

Waiting on #4189 to land. 👌

[pchaigno]

It looks like the YAML file doesn't support candidates (which is fine since we don't use that anymore). Thus, the above line can be simplified, as well as the additional logic in matches? to handle candidates.

[smola]

I'll perform this change next time I get to work on this.

[pchaigno]

Actually, there's a test for that in test_pedantic.rb, test_heuristics_are_sorted; could you update it to check the YAML file instead? We poor humans have a hard time keeping things alphabetized (speaking from experience :p).

[smola]

Updated the test.

[pchaigno]

Could you remove these? I'll open an issue for the first one so we don't forget and I don't think we supported the second one before anyway. We're very likely to forget TODOs at the end of the file, even if we later fix them.

[smola]

Oh, I forgot about these.

Actually, there is currently a disambiguation rule that returns two languages: [Language["Linux Kernel Module"], Language["AMPL"]] for .mod.

[smola]

There is still one case I couldn't convert to a plain regular expression:

https://github.com/github/linguist/blob/ed44c0b86d317b753c607742b137dce9594bed97/lib/linguist/heuristics.rb#L326

However, I found no file that is misclassified with something simpler such as:

(?<!\S)\.(include|globa?l)\s|(?<!\/\*)(\A|\n)\s*\.[A-Za-z][_A-Za-z0-9]*:

According to @Alhadis here 06c049b this is the rationale:

This commit modifies the heuristic to assume the following flow:

1. If a line contains ".include" or ".global"/".globl" which doesn't
   follow a non-whitespace character, assume GAS.
2. Otherwise, if the line starts with a command like ".LG7E0" with a
   possible string of whitespace before it, assume it's also GAS.

UNLESS either of the following conditions are true:
2a. The token is enclosed by a string or /* multiline comment */
2b. The previous line ends with a backslash to denote a statement
broken between lines, with possible whitespace and/or comment
sequences between the backslash and the actual newline.

3. If neither of the above are met, assume the file is MAXScript.

It seems usage of multiline comments with /* */ syntax are rare in Unix Assembly. Supported by HLA Syntax but I still didn't see any file with .ms extension using them.

If we assumed this kind of comments are not there, we can just simplify the expression to look for a label without any preceding /* (in multi-line mode). What do you think?

[Alhadis]

If we assumed this kind of comments are not there, we can just simplify the expression to look for a label without any preceding /* (in multi-line mode). What do you think?

That would work. I was probably being overly cautious with accuracy considering the PR was my first real contribution to Linguist (and the open-ended fallback to MAXScript convinced me that keeping matches airtight was a wise thing to do).

I don't remember seeing any GAS files with multi-line /* */ comments, however.

[smola]

@Alhadis 👍 Created a PR simplifying it: #4224

I'll change this PR to support combining negative and positive patterns at the same time. And also supporting multiple patterns with && instead of ||. The later also affects QMake, which was originally data.include?("HEADERS") && data.include?("SOURCES") but was effectively changed to data.include?("HEADERS") || data.include?("SOURCES")

[smola]

Now with the support of and: blocks in heuristic.yml, functionality matches heuristics.rb behavior.
Extensions for QMake and .ms were the remaining ones, fixed in this iteration.

[pchaigno]

LGTM! Thanks @smola!

[vmg]

This looks great to me. I'm not even sure who has the final say on these PRs these days, but if nobody is opposed we can go ahead and merge. :)

[pchaigno]

I'm not even sure who has the final say on these PRs these days

You and I.

[pchaigno]

And, just like that, I broke @Alhadis' pull request (cf. #4258). Sorry 😛

[Alhadis]

Lovely. Now what am I gonna do? 😕 Change the YAML parsing logic to accept a filename property?

[pchaigno]

@Alhadis I was thinking we could add something along the lines of filename_pattern:?

[Alhadis]

Alright, I'll add it. But this really needs to be more object-oriented, IMHO.

For example, rather than specifying filename_pattern or negative_pattern as separate fields, we should have them as properties of a single Pattern object:

pattern:
- source: '/^foo/'
  target: data
- source: '/^bar'
  target: name
- source: '^qux'
  negated: true

Strings could still be used; simply treated as shorthand for an object with only the source field defined:

pattern: '/^foo/'

# Which is shorthand for:
pattern:
- source: '/^foo/'
  negated: false
  target: data

That way, we could adopt a cleaner implementation in heuristics.rb by subclassing Regexp and storing rule-specific properties on instances (such as whether to test against a file's data or its name).

This would also solve the problem of not being able to specify modifier flags for the regular expression, such as /i or /x (the latter would be useful if very lengthy expressions need to be defined in the YML file.