Cross-library fragile base class problem

[nddrylliog]

I'm murdering my way through the old rock codebase (it's happening in branch issue524, for those who don't follow), and there's many bugs that were fixed, but something's still amiss.

Here's what happened previously:

• you're working on "foo", which has its source in "$OOC_LIBS/foo/source"
• your project uses lib "bar", which has its source in "$OOC_LIBS/bar/source"
• rock identifies both those 'projects' by the pathElement's name "source" => they're both stored in the same SourceFolder, and in the same static lib archive source-[target].a
• dependency analysis is done independently for each SourceFolder, which means if one of foo's classes extend on of bar's classes, and that class from Bar changes structures (add/reorder methods/fields), foo's class will be recompiled as well, to accommodate to the new structure.

Here's what happens now:

• you're working on "foo" and "bar", as previously described
• rock identifies those as two separate SourceFolder, named respectively "foo" and "bar".
• analysis is done separately for both those SourceFolder so if a class from "bar" changes structure, "foo"'s classes won't get recompiled.

So I'm thinking of clearer data structures to figure out exactly what changed and how we manage that strategy.

Anyway, it's going to be a wild ride, but rock is getting cleaner, heinous commit by heinous commit!

(That's a free Sonic 2 image for you, I don't know why GitHub decided to upload the content of my clipboard but I'm glad it's not more embarassing).

[alexnask]

Good to hear!
I'm gonna work on rock some more too after my exams (should be finishing up in early February), I am busy as hell now unfortunately...
Mayeb I'll do a couple of all-nighters on weekends till then (no promises) though.

[nddrylliog]

Alrighty in fact the problem doesn't seem too hard to fix anymore in rock's 95x branch (which is pretty much the edge of clean rock code anyway).

Once you realize that structuralDirties is local to a single Archive, the problem is easy to grasp:

https://github.com/nddrylliog/rock/blob/be672857c6528f7b56d5047706cddff2414c9754/source/rock/frontend/drivers/Archive.ooc#L241

I'm pretty sure Archive (or some other class) could have a map from module to Archive to make sure we query the right 'structuralDirties' list.

In fact, responsibilities probably need to be decoupled some more - knowing what depends on what and what needs to be recompiled seems to be overboard for an Archive class - but it could be made to work in the short term with a not-so-horrible solution in Archive.

[nddrylliog]

Looks like a biggie, post-poning to 0.9.8.

[nddrylliog]

Test case

SourceFolders

liba

A liba folder in $OOC_LIBS, with liba.use:

Name: liba
Version: 0.1
SourcePath: source

And source/liba/klass.ooc:

Klass: class {
    a := 1
    b := 2
    c := 3
}

projectb

A projectb folder in $OOC_LIBS, with projectb.use:

Name: projectb
Version: 0.1
SourcePath: source
Main: projectb/main

And source/projectb/main.ooc:

use liba
import liba/klass

Klazz: class extends Klass {
    init: func {
        "a, b, c = #{a}, #{b}, #{c}" println()
    }
}

Klazz new()

Running it

Follow the steps:

• cd into projectb, run rock -r -v --debuglibcache (last two arguments are optional).
• edit liba/source/liba/klass.ooc, swap the declarations of b and c
• run rock again from projectb

Expected results

The compiled program should display '1, 2, 3' both times

Observed results

Before the fix, it displays '1, 2, 3' first, and then '1, 3, 2'.

After the fix, the observed result is as expected.

[nddrylliog]

The fix

As I mentioned earlier, the idea is that dirtyModules is local to an Archive, and they're collected by the SequenceDriver, when it generates C sources.

Even though there was code to mark modules as dirty in case imports had structural changes, it only worked between two modules of the same SourceFolder, not across them:

dirtyModules := ArrayList<Module> new()
structuralDirties := ArrayList<Module> new()

// `modules` is all modules in the Archive / SourceFolder
for (module in modules) {
  // some logic to mark some modules as structuralDirties directly

  ImportClassifier classify(module)
  for(imp in module getAllImports()) {
      candidate := imp getModule()
      if(structuralDirties contains?(candidate)) {
          // at this point, module is dirty because it imports candidate
      }
  }

To make it work across them, one has to use the map class variable of Archive to retrieve the right set of structuralDirties. However, structuralDirties is not an instance variable - it is merely a temporary variable used by the dirtyModules() method to do its logic.

If we're going to have structuralDirties as an instance variable of the Archive class, we have to be careful in which order we check for structuralDirties. In our case, if we check for projectb first, then for liba, the changes from liba would be detected too late, and projectb's modules would be marked fresh before even knowing if liba had changes.

So we need a way to know in which order we should check for modules... thankfully, since February 2013, we have rock/frontend/drivers/DependencyGraph, which does just that for us.

In that case, the generated graph looks something like this:

projectb -> sdk
projectb -> liba
liba -> sdk

Which is reduced to a list:

projectb, liba, sdk

This it the correct order for linker flags, e.g.:

gcc .libs/projectb-linux64.a .libs/liba-linux64.a .libs/sdk-linux64.a -o projectb

As for us, it's exactly the reverse of what we need. If we check dependencies in sdk first, then in liba, then in projectb, structural changes will be propagated and the right modules will be recompiled.

Implementation

For implementation details, I'll let you look at the diff. It's still pretty elegant - we could probably refactor to get rid of the big ugly 'map', 'dirtyModules' and 'structuralDirties' instance variables in Archive, but for our current purposes, they don't hurt a lot, except my pride.

Afterword

Could this be the end of all libcache woes? That would be pretty neat. When 0.9.8 is out, we should encourage people to not clean everytime, to see if there are still edge cases - but I doubt it.

The libcache system was pretty well thought out, then cleaned up in 0.9.5, and the fact that such a simple change could fix it completely boggles the mind. I'm happy though :)