Scrooge does not appear to honor Apache Thrift's list of reserved words

[rklancer]

Expected behavior

Apache Thrift defines a list of reserved words that apparently cannot be used as field identifiers: https://git-wip-us.apache.org/repos/asf?p=thrift.git;a=blob;f=compiler/cpp/src/thrift/thriftl.ll;hb=e1832c354391deb0e0ce94a62ff32e8ce1c83fd3#l273

Therefore, I expect Scrooge to fail to compile a .thrift file with a field named delete. Failing that I expect a clear warning that the thrift file may be incompatible with other Thrift implementations.

Actual behavior

We have been using a Scrooge-compiled Thrift file in a production Java system for a while:

struct ThingMessage {
  //...
  200: optional bool delete
  //...

However, we are unable to use Apache Thrift's python generator to build an ops tool to observe these messages:

thrift --gen py -I .../ThingMessage.thrift:35
[ERROR: .../ThingMessage.thrift:35] (last token was 'delete')
Cannot use reserved language keyword: "delete"

[rklancer]

Smells like this could be an intentional choice, of course. Let me know.

That said, there's little or no public documentation of Thrift's reserved words, so users aren't likely to realize they're transgressing against compatibility unless the compiler either refuses to compile or at least requires them to use some kind of "--force" flag to override compatibility.

[kevinoliver]

it'd be good if we matched apache thrift's behavior.

i suspect if we didn't give users an escape valve it'd wreck havoc on them. perhaps via a "comment" to allow specific keywords within the file?

[mosesn]

@rklancer would you be open to making a PR that reserves the thrift reserved words?

[rtyley]

The new checks for reserved Thrift keywords introduced by 884f360:

scrooge/scrooge-generator/src/main/scala/com/twitter/scrooge/frontend/ThriftParser.scala

Lines 110 to 115 in a647e1d

	lazy val simpleID: Parser[SimpleID] = positioned(simpleIDRegex ^^ { x =>
	if (ThriftKeywords.contains(x))
	throw new KeywordException(x)
	SimpleID(x)
	})

...do not seem to be disabled by the scroogeDisableStrict flag:

scrooge/scrooge-sbt-plugin/src/main/scala/com/twitter/ScroogeSBT.scala

Lines 80 to 83 in a647e1d

	val scroogeDisableStrict = SettingKey[Boolean](
	"scrooge-disable-strict",
	"issue warnings on non-severe parse errors instead of aborting"
	)

...just checking, was that intentional?

[emdash-ie]

was that intentional?

I believe it was intentional – see 3376785, which says:

Problem:
We want to align internal thrift with OSS standards.

Solution:
To ensure no new usages of reserved keywords arise,
we'll change the ThriftParser from optionally failing or
warning a user that they've used a keyword to always failing.