`rustc-tests` test suite failure

[RyanGlScott]

The rustc-tests test suite current fails with:

$ cabal run rustc-tests

The version of `rustc' the tests will try to use is older than 1 month
info: using existing install for 'nightly-2018-04-19-x86_64-unknown-linux-gnu'
info: override toolchain for '/home/ryanglscott/Documents/Hacking/Haskell/language-rust' set to 'nightly-2018-04-19-x86_64-unknown-linux-gnu'

  nightly-2018-04-19-x86_64-unknown-linux-gnu unchanged - rustc 1.27.0-nightly (ac3c2288f 2018-04-18)

Running tests with rustc 1.27.0-nightly (ac3c2288f 2018-04-18)

attributes.rs: [OK]
items.rs: [OK]
types.rs: [OK]
precedences.rs: [OK]
expressions.rs: [Failed]
ERROR (Parsing using our parser): parse failure at 51:15 (Syntax error: unexpected `return' (expected a literal, an identifier, '&&', '&', '(', '-', '::', '<', '<<', '[', '_', Self, auto, box, catch, crate, default, dyn, mut, ref, self, super, or union))
literals.rs: [OK]
statement-expressions.rs: [OK]
patterns.rs: [OK]
macros.rs: [OK]
statements.rs: [OK]
empty.rs: [OK]
let.rs: [OK]

         Difference tests  Total 
 Passed  11                11    
 Failed  1                 1     
 Total   12                12

The failing test is this:

language-rust/sample-sources/expressions.rs

Line 51 in 82acd31

let r#return = 0;

Perhaps we simply need to bump the rustc version used in the test suite?

[RyanGlScott]

Actually, the same test case passes using the version of the code hosted at https://github.com/harpocrates/language-rust, at commit harpocrates@9d509c4 (plus backporting f1b4a92 in order to make rustc-tests compile with GHC 9.4.8). Some more investigation is required to figure out why this test case is only happening in the fork here...

[RyanGlScott]

Bisecting reveals that this is a regression introduced in 9ff9176.

[RyanGlScott]

Ah, I think this is because we check if an Ident is a keyword (such as return) here:

language-rust/src/Language/Rust/Pretty/Resolve.hs

Lines 227 to 252 in 82acd31

	-- | An identifier can be invalid if
	--
	-- * it does not lex into an identifier
	-- * it is a keyword
	--
	resolveIdent :: Ident -> ResolveM Ident
	resolveIdent i@(Ident { name = s, raw = r }) =
	scope i $ case toks of
	Right [Spanned (IdentTok i') _]
	| i /= i' -> err i ("identifier `" ++ s ++ "' does not lex properly")
	| i `elem` keywords && not r -> do
	correct ("identifier `" ++ s ++ "' is a keyword")
	pure i{ raw = True }
	| otherwise -> pure i
	_ -> err i ("identifier `" ++ s ++ "' does not lex properly")
	where
	keywords = map mkIdent $ words "as box break const continue crate else enum extern false fn for\
	\ if impl in let loop match mod move mut pub ref return Self self\
	\ static struct super trait true type unsafe use where while\
	\ abstract alignof become do final macro offsetof override priv\
	\ proc pure sizeof typeof unsized virtual yield"
	s' = (if r then "r#" else "") ++ s
	toks = execParser (lexTokens lexToken) (inputStreamFromString s') initPos

Note the i `elem` keywords bit. Before 9ff9176, the identifier r#return would return False on this check (as expected), since r#return's raw value would be True but mkIdent "return"'s raw value would be False. After 9ff9176, however, raw values are no longer taken into account when checking Idents for equality, so r#return is deemed to be equal to mkIdent "return". This causes the parse to mistakenly flag r#return as a keyword.

@yav, do you have any thoughts on how best to proceed? I am inclined to simply revert 9ff9176, but I am unclear if there is another motivation for this commit.

[yav]

I think x and #x really are the same identifier (i.e., you can't have two different variables with those names) so I don't think we should revert 9fff176. As far as I understand the situation, I think a reasonable fix would be to just do the test on the name of the identifier, so replace i with s and remove the map mkIdent from the keywords

[RyanGlScott]

Actually, my assessment is off: the resolveIdent function isn't involved at all here. (I do think it's worth applying the refactoring you suggest, however.) I'll keep digging to find the real cause.

[RyanGlScott]

The bug is much more subtle than I realized, as the Eq Ident instance is being used by the happy-generated parser. Can you spot where it's being used?

language-rust/src/Language/Rust/Parser/Internal.y

Line 196 in 82acd31

return { Spanned (IdentTok "return") _ }

As it turns out, it's being used in the "return" part! This is because the field of IdentTok is of type Ident, and because the parser file has OverloadedStrings enabled, "return" is an overloaded string pattern that matches against a value of type Ident using the Eq and IsString instances for Ident. (Note that the IsString Ident instance makes a non-raw identifier, i.e., where raw = False.)

The intent here is to match against return keywords (i.e., where raw = False), but because the Eq Ident keyword ignores raw, this will match both return and r#return. This is bad news for when we try to use r#return as an identifier, as the parser will reject it as though it were a return keyword.

---

One possible way to fix this is by changing every pattern to explicitly match against raw = False:

diff --git a/src/Language/Rust/Parser/Internal.y b/src/Language/Rust/Parser/Internal.y
index 5b0482e..36e5fff 100644
--- a/src/Language/Rust/Parser/Internal.y
+++ b/src/Language/Rust/Parser/Internal.y
@@ -170,74 +170,74 @@ import qualified Data.List.NonEmpty as N
   rawByteStr     { Spanned (LiteralTok ByteStrRawTok{} _) _ }
 
   -- Strict keywords used in the language
-  as             { Spanned (IdentTok "as") _ }
-  box            { Spanned (IdentTok "box") _ }
-  break          { Spanned (IdentTok "break") _ }
-  const          { Spanned (IdentTok "const") _ }
-  continue       { Spanned (IdentTok "continue") _ }
-  crate          { Spanned (IdentTok "crate") _ }
-  else           { Spanned (IdentTok "else") _ }
-  enum           { Spanned (IdentTok "enum") _ }
-  extern         { Spanned (IdentTok "extern") _ }
-  false          { Spanned (IdentTok "false") _ }
-  fn             { Spanned (IdentTok "fn") _ }
-  for            { Spanned (IdentTok "for") _ }
-  if             { Spanned (IdentTok "if") _ }
-  impl           { Spanned (IdentTok "impl") _ }
-  in             { Spanned (IdentTok "in") _ }
-  let            { Spanned (IdentTok "let") _ }
-  loop           { Spanned (IdentTok "loop") _ }
-  match          { Spanned (IdentTok "match") _ }
-  mod            { Spanned (IdentTok "mod") _ }
-  move           { Spanned (IdentTok "move") _ }
-  mut            { Spanned (IdentTok "mut") _ }
-  pub            { Spanned (IdentTok "pub") _ }
-  ref            { Spanned (IdentTok "ref") _ }
-  return         { Spanned (IdentTok "return") _ }
-  Self           { Spanned (IdentTok "Self") _ }
-  self           { Spanned (IdentTok "self") _ }
-  static         { Spanned (IdentTok "static") _ }
-  struct         { Spanned (IdentTok "struct") _ }
-  super          { Spanned (IdentTok "super") _ }
-  trait          { Spanned (IdentTok "trait") _ }
-  true           { Spanned (IdentTok "true") _ }
-  type           { Spanned (IdentTok "type") _ }
-  unsafe         { Spanned (IdentTok "unsafe") _ }
-  use            { Spanned (IdentTok "use") _ }
-  where          { Spanned (IdentTok "where") _ }
-  while          { Spanned (IdentTok "while") _ }
-  do             { Spanned (IdentTok "do") _ }
+  as             { Spanned (IdentTok (Ident { name = "as", raw = False })) _ }
+  box            { Spanned (IdentTok (Ident { name = "box", raw = False })) _ }
+  break          { Spanned (IdentTok (Ident { name = "break", raw = False })) _ }
+  const          { Spanned (IdentTok (Ident { name = "const", raw = False })) _ }
+  continue       { Spanned (IdentTok (Ident { name = "continue", raw = False })) _ }
+  crate          { Spanned (IdentTok (Ident { name = "crate", raw = False })) _ }
+  else           { Spanned (IdentTok (Ident { name = "else", raw = False })) _ }
+  enum           { Spanned (IdentTok (Ident { name = "enum", raw = False })) _ }
+  extern         { Spanned (IdentTok (Ident { name = "extern", raw = False })) _ }
+  false          { Spanned (IdentTok (Ident { name = "false", raw = False })) _ }
+  fn             { Spanned (IdentTok (Ident { name = "fn", raw = False })) _ }
+  for            { Spanned (IdentTok (Ident { name = "for", raw = False })) _ }
+  if             { Spanned (IdentTok (Ident { name = "if", raw = False })) _ }
+  impl           { Spanned (IdentTok (Ident { name = "impl", raw = False })) _ }
+  in             { Spanned (IdentTok (Ident { name = "in", raw = False })) _ }
+  let            { Spanned (IdentTok (Ident { name = "let", raw = False })) _ }
+  loop           { Spanned (IdentTok (Ident { name = "loop", raw = False })) _ }
+  match          { Spanned (IdentTok (Ident { name = "match", raw = False })) _ }
+  mod            { Spanned (IdentTok (Ident { name = "mod", raw = False })) _ }
+  move           { Spanned (IdentTok (Ident { name = "move", raw = False })) _ }
+  mut            { Spanned (IdentTok (Ident { name = "mut", raw = False })) _ }
+  pub            { Spanned (IdentTok (Ident { name = "pub", raw = False })) _ }
+  ref            { Spanned (IdentTok (Ident { name = "ref", raw = False })) _ }
+  return         { Spanned (IdentTok (Ident { name = "return", raw = False })) _ }
+  Self           { Spanned (IdentTok (Ident { name = "Self", raw = False })) _ }
+  self           { Spanned (IdentTok (Ident { name = "self", raw = False })) _ }
+  static         { Spanned (IdentTok (Ident { name = "static", raw = False })) _ }
+  struct         { Spanned (IdentTok (Ident { name = "struct", raw = False })) _ }
+  super          { Spanned (IdentTok (Ident { name = "super", raw = False })) _ }
+  trait          { Spanned (IdentTok (Ident { name = "trait", raw = False })) _ }
+  true           { Spanned (IdentTok (Ident { name = "true", raw = False })) _ }
+  type           { Spanned (IdentTok (Ident { name = "type", raw = False })) _ }
+  unsafe         { Spanned (IdentTok (Ident { name = "unsafe", raw = False })) _ }
+  use            { Spanned (IdentTok (Ident { name = "use", raw = False })) _ }
+  where          { Spanned (IdentTok (Ident { name = "where", raw = False })) _ }
+  while          { Spanned (IdentTok (Ident { name = "while", raw = False })) _ }
+  do             { Spanned (IdentTok (Ident { name = "do", raw = False })) _ }
 
   -- Keywords reserved for future use
-  abstract       { Spanned (IdentTok "abstract") _ }
-  alignof        { Spanned (IdentTok "alignof") _ }
-  become         { Spanned (IdentTok "become") _ }
-  final          { Spanned (IdentTok "final") _ }
-  macro          { Spanned (IdentTok "macro") _ }
-  offsetof       { Spanned (IdentTok "offsetof") _ }
-  override       { Spanned (IdentTok "override") _ }
-  priv           { Spanned (IdentTok "priv") _ }
-  proc           { Spanned (IdentTok "proc") _ }
-  pure           { Spanned (IdentTok "pure") _ }
-  sizeof         { Spanned (IdentTok "sizeof") _ }
-  typeof         { Spanned (IdentTok "typeof") _ }
-  unsized        { Spanned (IdentTok "unsized") _ }
-  virtual        { Spanned (IdentTok "virtual") _ }
+  abstract       { Spanned (IdentTok (Ident { name = "abstract", raw = False })) _ }
+  alignof        { Spanned (IdentTok (Ident { name = "alignof", raw = False })) _ }
+  become         { Spanned (IdentTok (Ident { name = "become", raw = False })) _ }
+  final          { Spanned (IdentTok (Ident { name = "final", raw = False })) _ }
+  macro          { Spanned (IdentTok (Ident { name = "macro", raw = False })) _ }
+  offsetof       { Spanned (IdentTok (Ident { name = "offsetof", raw = False })) _ }
+  override       { Spanned (IdentTok (Ident { name = "override", raw = False })) _ }
+  priv           { Spanned (IdentTok (Ident { name = "priv", raw = False })) _ }
+  proc           { Spanned (IdentTok (Ident { name = "proc", raw = False })) _ }
+  pure           { Spanned (IdentTok (Ident { name = "pure", raw = False })) _ }
+  sizeof         { Spanned (IdentTok (Ident { name = "sizeof", raw = False })) _ }
+  typeof         { Spanned (IdentTok (Ident { name = "typeof", raw = False })) _ }
+  unsized        { Spanned (IdentTok (Ident { name = "unsized", raw = False })) _ }
+  virtual        { Spanned (IdentTok (Ident { name = "virtual", raw = False })) _ }
 
   -- Weak keywords, have special meaning only in specific contexts.
-  default        { Spanned (IdentTok "default") _ }
-  union          { Spanned (IdentTok "union") _ }
-  catch          { Spanned (IdentTok "catch") _ }
-  auto           { Spanned (IdentTok "auto") _ }
-  yield          { Spanned (IdentTok "yield") _ }
-  dyn            { Spanned (IdentTok "dyn") _ }
+  default        { Spanned (IdentTok (Ident { name = "default", raw = False })) _ }
+  union          { Spanned (IdentTok (Ident { name = "union", raw = False })) _ }
+  catch          { Spanned (IdentTok (Ident { name = "catch", raw = False })) _ }
+  auto           { Spanned (IdentTok (Ident { name = "auto", raw = False })) _ }
+  yield          { Spanned (IdentTok (Ident { name = "yield", raw = False })) _ }
+  dyn            { Spanned (IdentTok (Ident { name = "dyn", raw = False })) _ }
 
   -- Comments
   outerDoc       { Spanned (Doc _ Outer _) _ }
   innerDoc       { Spanned (Doc _ Inner _) _ }
 
   -- Identifiers.
-  '_'            { Spanned (IdentTok "_") _ }
+  '_'            { Spanned (IdentTok (Ident { name = "_", raw = False })) _ }
   IDENT          { Spanned IdentTok{} _ }
 
   -- Lifetimes.

This is rather tiresome, however. I still can't help but wonder if it would be better to make the Eq instance account for raw, but offer a separate equality function over Idents that ignores raw values. What do you think, @yav?

[RyanGlScott]

A third option would be to define a pattern synonym:

pattern Keyword :: Name -> Ident
pattern Keyword n <- Ident { name = n, raw = False }
  where
    Keyword = mkIdent

And use it everywhere where we currently pattern-match on Ident { ..., raw = False }.

[yav]

Hm, I see. My gut feeling is that we should fix this in the lexer to produce a different token for keywords---it should be the one distinguishing keywords from identifiers, and we shouldn't have to worry about the # later, except when pretty printing. An added benefit of doing this in the lexer, is that it would make it possible to write a syntax highlighter by just running the lexer, and it would know what's a keyword vs. identifier.

[RyanGlScott]

My gut feeling is that we should fix this in the lexer to produce a different token for keywords---it should be the one distinguishing keywords from identifiers

For what it's worth, the fact that the lexer doesn't distinguish keywords from other identifiers is an intentional choice. language-rust notes here that it tries to mimic rustc's own lexer as much as possible. Sure enough, rustc also lexes keywords and other identifiers identically (see here), and it only distinguishes them during parsing.

Of course, we could deviate from this part of rustc if we chose, although we would need to update the comments to state as much.

[yav]

Well, not sure. For what it's worth, I don't think the Cryptol to Rust compiler relies on the equality change anymore, so we could just revert it, and put a note in the Identifier module to indicate that these are supposed to be very syntactic and so #x and x are not equal by default.

[RyanGlScott]

I am leaning towards reverting the commit. I'll also make sure to expand the comments near Ident to state that the Eq and Ord instances take rawness into account.