Please, make it easier to add new rules

[snosov1]

Hi!

In a pretty recent thread Mr. Bright complains about the number of compilation errors in a 2 years old D code base. A perfect answer to him would be "Please, run dfix on the code base and you're done". However, currently, dfix is not in shape to solve the problem.

But the saddest part is that the dfix solution is completely ignored by most of the people in the thread. They're not talking like "We should implement more rules in dfix", but rather argue about git log -S solutions and similar unstructured approaches.

I wanted to try and make a positive change in that thread by stating something like - "Hey, I've just made a pull request with a rule to dfix that changes all the occurences of 'fnmatch' with 'globMatch'". I expected this to be rather trivial, because it is indeed pretty easy to come up with a "grep"-based, for example.

However, looking at the code now, I don't have a clue how to do that. Not only there's no documentation on how to add new rules, but you can't really figure this out looking at the code as well.

Probably, this is the main reason why dfix is not catching up. If adding a rule for something like fnmatch->globMatch change would be as trivial as adding a 10-lines function, then making a code-breaking change to Phobos or compiler without adding this rule could be considered a public crime.

So, I wonder, is restructuring the code to a better shape and providing a documentation on adding new rules is it too much to ask?

P.S. Probably, you've heard of flint - a C++ linting tool written in D from Facebook, that was open-sourced by Andrei some time ago. I think it makes a good example of similar "rule"-based software. Consider a simple rule https://github.com/facebook/flint/blob/master/Checks.d#L476

uint checkBlacklistedIdentifiers(const string fpath, const CppLexer.Token[] v) {
  uint result = 0;

  string[string] banned = [
    "strtok" :
      "strtok() is not thread safe, and has safer alternatives.  Consider "
      "folly::split or strtok_r as appropriate.\n"
  ];

  foreach (ref t; v) {
    if (t.type_ != tk!"identifier") continue;
    auto mapIt = t.value_ in banned;
    if (!mapIt) continue;
    lintError(t, *mapIt);
    ++result;
  }

  return result;
}    

The code is so straightforward, you don't even need a documentation for it.

Granted, modifying the code can be a lot trickier, then just examining it, but at least simple things like fnmatch->globMatch can be made a lot easier (I think). And as Walter points out in the thread, even those simple things can make a big difference.

[Hackerpilot]

I don't think you understand the difference between dfix and dscanner. dfix is not a lint tool.

[Hackerpilot]

The existing dscanner rules can be found here: https://github.com/Hackerpilot/Dscanner/tree/master/src/analysis

[snosov1]

=) I think I do understand that dfix is not a lint tool. However, let's make sure we're on the same page.

As I understand, dfix is supposed to be an analogue of the go fix tool for the D programming language. The main purpose of go fix is to help transition between different compiler versions. In Go 1.0 they announced backward compatibility guarantee, but for a good amount of time prior to 1.0 they were free to break user's code.

And one of the reasons it was not that bad is because you could run go fix after compiler update, and it would change your code to compile with the current version preserving the semantics.

Despite all the talking, D still breaks user's code with almost every release, for good reasons or bad. I'm not arguing that it's necessarily a bad thing, but it's a fact, that you have to deal with. The whole thread was started not only because you cannot longer compile a 2 years old code, but you don't even have a clue how to fix it.

So, dfix (in a go fix sense) should also make the transitions between compiler versions smoother.

Assuming, that I guessed it right, you can indeed treat dfix as a "rule"-based program - it scans through the code and tries to apply some "rules" to it. In case of a linter - it checks for styling and similar issues and reports errors. In case of dfix, currently, it also checks mostly the styling issues, but fixes the errors in place instead of reporting them.

The difference is not that big - linting tools report the errors, but "fix" tools are supposed to correct those instead of reporting.

Having dfix, a process for a breaking change to e.g. Phobos could demand that an appropriate rule is added to dfix. For example, someone submits a PR changing the name fnmatch to globMatch. Everybody decides that it's a good change, but, of course, it's a breaking one. So, the reviewer says that it will be merged only after the appropriate rule is added to dfix. The author sends a PR with something like (pseudo-code):

uint fixRenames(const string fpath, const Token[] input, Token[] output) {
    uint result = 0;

    string[string] renamed = [
        "fnmatch" : "globMatch"
        ];

    foreach (ref t; v) {
        if (t.type_ != tk!"fun_call")
        {
            output ~= t;
            continue;
        }

        auto mapIt = t.value_ in renamed;
        if (!mapIt)
        {
            output ~= t;
            continue;
        }

        output ~= *mapIt;
        ++result;
    }

    return result;
}

Currently, dfix has one grand function that Does Everything Right. So, it's unreasonable to ask the original PR author to add a new rule, because it's far from trivial. But demanding a function similar to the above for a breaking change is completely reasonable.

I understand that the fnmatch->globMatch example is quite simple and that other cases would probably be more involved. But at the moment even the fnmatch->globMatch rule can't be implemented easily, which is discouraging to the contributors. And probably one of the reasons, why people in the thread are thinking of "warnings" and "suggestive messages" instead of using dfix approach.

[Hackerpilot]

dfix is written the way it is because I had the following goals:

• Fix changes in the language
• Don't change the existing code formatting

I had no plans to implement automating library upgrades for the following reasons:

• There's no way to figure out what the right thing to do is if a function's parameters change
• Renaming a function requires that you know not just the function's name, but also which module it comes from and figuring out exactly which function is actually being called in the source being upgraded. You can't just rename all "foo" to "bar" if two modules both declare a "foo".
• Mixins will guarantee that you can't always get the right results.

Back to the "preserving semantics" thing, the only way to do that correctly is by having the full compiler front-end do the rewrites for you. And even then Go has things easy because they have a standard formatting tool and if you do a code -> ast -> code transformation you won't have people complaining about the whitespace not being preserved.

[snosov1]

dfix is written the way it is because I had the following goals:

Ok, I see. I didn't really make a distinction between the compiler and the
library in my head. Now that I'm thinking about that, it seems that library
changes can indeed be trickier.

There's no way to figure out what the right thing to do is if a function's parameters change

I would think that a "new" function will often provide a superset of the
"old" functionality. It's not always so, but I think it should be so
quite often, and if it's not the case - maybe the change should be
re-considered.

Renaming a function requires that you know not just the function's name, but also which module it comes from and figuring out exactly which function is actually being called in the source being upgraded. You can't just rename all "foo" to "bar" if two modules both declare a "foo".

Currently, DCD can provide something like "goto definition". If i get you
right - it will provide the list of possible candidates, but it won't be
able to resolve ambiguities. But maybe a case when there's only one
function is important enough on itself?

Mixins will guarantee that you can't always get the right results.
Back to the "preserving semantics" thing, the only way to do that correctly is by having the full compiler front-end do the rewrites for you. And even then Go has things easy because they have a standard formatting tool and if you do a code -> ast -> code transformation you won't have people complaining about the whitespace not being preserved.

Yeah, those are the problems you cannot solve easily.

But still, it seems dfix can at least solve part of the problems. If
somebody gets a hundred of errors during compilation and after dfix it
becomes a dozen - probably it's not that bad after all.

Best regards,
Sergei Nosov

On Tue, Dec 2, 2014 at 5:48 PM, Brian Schott notifications@github.com
wrote:

dfix is written the way it is because I had the following goals:

• Fix changes in the language
• Don't change the existing code formatting

I had no plans to implement automating library upgrades for the following
reasons:

• There's no way to figure out what the right thing to do is if a
  function's parameters change
• Renaming a function requires that you know not just the function's
  name, but also which module it comes from and figuring out exactly which
  function is actually being called in the source being upgraded. You can't
  just rename all "foo" to "bar" if two modules both declare a "foo".
• Mixins will guarantee that you can't always get the right results.

Back to the "preserving semantics" thing, the only way to do that
correctly is by having the full compiler front-end do the rewrites for you.
And even then Go has things easy because they have a standard formatting
tool and if you do a code -> ast -> code transformation you won't have
people complaining about the whitespace not being preserved.

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