Constants-merging produces incorrect top-level module

[jparise]

I believe the constants-merging code from #224 might be breaking the generation of code that follows this pattern:

1. a.thrift defines a bunch of things, including structs and a service.
2. a.thrift also includes b.thrift.
3. b.thrift defines some constants.

This results in the top-level module produced by a.thrift (A) containing only the constants from b.thrift.

// a.thrift
include "b.thrift"

struct Foo {
    1: string name
}

// b.thrift
const i8 Z = 26

This results in an a.ex (defmodule(A)) containing the constant and a foo.ex containing struct Foo. I would have expected the constant to be written to a b.ex (defmodule(B)) instead.

[dantswain]

@jparise I'm not able to reproduce this yet. I added the following test to binary_protocol_test.exs and it passes:

  @thrift_file name: "included_constants.thrift", contents: """
  const i8 Z = 26
  """

  @thrift_file name: "includes_constants.thrift", contents: """
  include "included_constants.thrift"

  struct IncludesConstants {
    1: string name
    2: i8 z = included_constants.Z
  }
  """

  thrift_test "including a file with constants" do
    assert 26 == IncludedConstants.z
    assert %IncludesConstants{z: 26} == %IncludesConstants{}
  end

(Also tried swapping the order of the two files - still passes)

[dantswain]

OK, I'm getting somewhere. I can get the includee to generate constant macros where it should not, but it's still not overwriting anything else. Updated test case:

  @thrift_file name: "included_constants.thrift", contents: """
  const i8 Z = 26
  """

  @thrift_file name: "includes_constants.thrift", contents: """
  include "included_constants.thrift"

  struct IncludesConstants {
    1: string name
    2: i8 z = included_constants.Z
  }
  """

  @thrift_file name: "also_includes.thrift", contents: """
  include "included_constants.thrift"

  struct SomeOtherName {
    1: i32 id
  }
  """

  thrift_test "including a file with constants" do
    assert 26 == IncludedConstants.z
    assert %IncludesConstants{z: 26} == %IncludesConstants{}
    assert_raise UndefinedFunctionError, fn -> 26 == IncludesConstants.z end
    assert %SomeOtherName{} == SomeOtherName.new()
    assert_raise UndefinedFunctionError, fn -> 26 == AlsoIncludes.z end
  end

The last assert_raise fails because AlsoIncludes.z is being defined.

[dantswain]

I can try to come up with a simple-ish fix here, but I think the underlying problem is more complex. We end up writing out code for every time that a particular file is parsed. It was never a problem before because the operation is basically idempotent - if we parsed a file that defined struct Foo we would write out a module for Foo and then if we included that file from another file we would end up overwriting it with the same Foo. It wasn't a problem before because the resulting module would be the same.

I think.

I'm still trying to fully grok the parse/generate code, but I know that if I put some debug printing in the generate functions, they get triggered multiple times with the same constants but originating from multiple files.

This could also explain what you're seeing, @jparise, it could depend on how the list of files gets ordered.

It seems like the real solution would be to make a single pass over the files and generate a single unified schema, but that would be a pretty big change. I think I can fix this particular problem by only generating constants when the initial_file in the FileGroup defines the constant. We may want to do that for all of the models?

[jparise]

I'm in the same situation with regard to fully understanding the parser and generator, to be honest. But I also definitely encountered the same issue where included files are (re)written multiple times based on the set of input files. You can see that explicitly when you run mix thrift.generate --verbose, which logs all of the output files as they're written.

I would like us to address this at a fundamental level, which may mean a redesign of the FileGroup concept, because we otherwise can't guarantee anything about the relationship between input files and how their output will be written, which relates to what we're seeing here.

[jparise]

Also, my more immediate concern is about the constants being written to the including file's module. In my test case, we wouldn't have written anything to that module anyway, so the "overwrite" case might be a distraction and ultimately more of a systematic issue with the way we (re)write output files.

[dantswain]

So what I'm hearing is "yes, there is an underlying issue here but let's at least try to fix the reported bug first"? I'm good with that as long as we make a ticket to capture the underlying issue.

…

On Feb 10, 2017, at 3:37 PM, Jon Parise ***@***.***> wrote: Also, my more immediate concern is about the constants being written to the including file's module. In my test case, we wouldn't have written anything to that module anyway, so the "overwrite" case might be a distraction and ultimately more of a systematic issue with the way we (re)write output files. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

[dantswain]

Closed by #230. Follow-up is #236.