search

tweag / ormolu

A formatter for Haskell source code
https://ormolu-live.tweag.io
Other
958 stars 83 forks source link

AST differs on comment after a magic comment #856

Closed mitchellwrosen closed 2 years ago

mitchellwrosen commented 2 years ago

Describe the bug

The AST differs when this file is formatted.

To Reproduce

Format this file: https://github.com/unisonweb/unison/blob/6992f245675926d2a5f5b2c2f17545e877c87678/parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs

--debug output

parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:180:1-29 Comment False ("-- 1) buffer up the component" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:181:1-61 Comment False ("-- 2) in the event that the component is complete, then what?" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:182:1-81 Comment False ("--  * can write component provided all of its dependency components are complete." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:183:1-33 Comment False ("--    if dependency not complete," :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:184:1-70 Comment False ("--    register yourself to be written when that dependency is complete" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:186:1-29 Comment False ("-- an entry for a single hash" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:188:5-72 Comment True ("-- First, you are waiting for the cycle to fill up with all elements" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:189:5-68 Comment False ("-- Then, you check: are all dependencies of the cycle in the db?" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:190:5-75 Comment False ("--   If yes: write yourself to database and trigger check of dependents" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:191:5-37 Comment False ("--   If no: just wait, do nothing" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:275:42-78 Comment True ("-- pure Cache.nullCache -- to disable" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:281:7-90 Comment False ("-- The v1 codebase interface has operations to read and write individual definitions" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:282:7-93 Comment False ("-- whereas the v2 codebase writes them as complete components.  These two fields buffer" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:283:7-80 Comment False ("-- the individual definitions until a complete component has been written." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:337:21-80 Comment False ("-- if size was previously set, it's expected to match size'." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:344:21-104 Comment False ("-- for the component element that's been passed in, add its dependencies to missing'" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:354:21-78 Comment False ("-- notify each of the dependencies that h depends on them." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:396:13-50 Comment False ("-- skip if it has already been flushed" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:415:26-34 Comment True ("-- update" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:420:19-74 Comment False ("-- it's never even been added, so there's nothing to do." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:494:17-83 Comment False ("-- check to see if root namespace hash has been externally modified" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:495:17-45 Comment False ("-- and reload it if necessary" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:531:13-85 Comment False ("-- todo: check to see if root namespace hash has been externally modified" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:532:13-92 Comment False ("-- and do something (merge?) it if necessary. But for now, we just overwrite it." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:542:13-53 Comment False ("-- branchHeadChanges      <- TQueue.newIO" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:543:13-75 Comment False ("-- (cancelWatch, watcher) <- Watch.watchDirectory' (v2dir root)" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:544:13-40 Comment False ("-- watcher1               <-" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:545:13-32 Comment False ("--   liftIO . forkIO" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:546:13-26 Comment False ("--   $ forever" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:547:13-21 Comment False ("--   $ do" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:548:13-75 Comment False ("--       -- void ignores the name and time of the changed file," :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:549:13-63 Comment False ("--       -- and assume 'unison.sqlite3' has changed" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:550:13-48 Comment False ("--       (filename, time) <- watcher" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:551:13-80 Comment False ("--       traceM $ \"SqliteCodebase.watcher \" ++ show (filename, time)" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:552:13-57 Comment False ("--       readTVarIO rootBranchCache >>= \\case" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:553:13-41 Comment False ("--         Nothing -> pure ()" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:554:13-40 Comment False ("--         Just (v, _) -> do" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:555:13-87 Comment False ("--           -- this use of `conn` in a separate thread may be problematic." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:556:13-92 Comment False ("--           -- hopefully sqlite will produce an obvious error message if it is." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:557:13-57 Comment False ("--           v' <- runDB conn Ops.dataVersion" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:558:13-40 Comment False ("--           if v /= v' then" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:559:13-37 Comment False ("--             atomically" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:560:13-101 Comment False ("--               . TQueue.enqueue branchHeadChanges =<< runDB conn Ops.loadRootCausalHash" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:561:13-37 Comment False ("--           else pure ()" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:563:13-57 Comment False ("--       -- case hashFromFilePath filePath of" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:564:13-76 Comment False ("--       --   Nothing -> failWith $ CantParseBranchHead filePath" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:565:13-35 Comment False ("--       --   Just h ->" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:566:13-89 Comment False ("--       --     atomically . TQueue.enqueue branchHeadChanges $ Branch.Hash h" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:567:13-47 Comment False ("-- -- smooth out intermediate queue" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:568:13-19 Comment False ("-- pure" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:569:13-53 Comment False ("--   ( cancelWatch >> killThread watcher1" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:570:13-84 Comment False ("--   , Set.fromList <$> Watch.collectUntilPause branchHeadChanges 400000" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:571:13-18 Comment False ("--   )" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:575:57-80 Comment True ("-- hold off on returning" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:576:29-48 Comment True ("-- returning nothing" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:579:11-82 Comment False ("-- if this blows up on cromulent hashes, then switch from `hashToHashId`" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:580:11-39 Comment False ("-- to one that returns Maybe." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:735:13-82 Comment False ("-- given that a Branch is shallow, it's really `CausalHash` that you'd" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:736:13-63 Comment False ("-- refer to to specify a full namespace w/ history." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:737:13-85 Comment False ("-- but do we want to be able to refer to a namespace without its history?" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:801:1-88 Comment False ("-- well one or the other. :zany_face: the thinking being that they wouldn't hash-collide" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:837:5-46 Comment False ("-- we want to use sync22 wherever possible" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:838:5-77 Comment False ("-- so for each branch, we'll check if it exists in the destination branch" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:839:5-66 Comment False ("-- or if it exists in the source branch, then we can sync22 it" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:840:5-48 Comment False ("-- oh god but we have to figure out the dbid" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:841:5-56 Comment False ("-- if it doesn't exist in the dest or source branch," :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:842:5-42 Comment False ("-- then just use putBranch to the dest" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:916:44-80 Comment True ("-- (we don't write to the src anyway)" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1022:3-25 Comment False ("-- set up the cache dir" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1025:3-113 Comment False ("-- Tickle the database before calling into `sqliteCodebase`; this covers the case that the database file either" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1026:3-119 Comment False ("-- doesn't exist at all or isn't a SQLite database file, but does not cover the case that the database file itself is" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1027:3-52 Comment False ("-- somehow corrupt, or not even a Unison database." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1028:3-4 Comment False ("--" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1029:3-120 Comment False ("-- FIXME it would probably make more sense to define some proper preconditions on `sqliteCodebase`, and perhaps update" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1030:3-113 Comment False ("-- its output type, which currently indicates the only way it can fail is with an `UnknownSchemaVersion` error." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1034:7-37 Comment False ("-- Unexpected error from sqlite" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1038:5-47 Comment False ("-- try to load the requested branch from it" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1040:7-54 Comment False ("-- no sub-branch was specified, so use the root." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1043:11-68 Comment False ("-- this NoRootBranch case should probably be an error too." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1050:7-47 Comment False ("-- load from a specific `ShortBranchHash`" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1067:1-120 Comment False ("-- Push a branch to a repo. Optionally attempt to set the branch as the new root, which fails if the branch is not after" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1068:1-21 Comment False ("-- the existing root." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1078:3-46 Comment False ("-- Pull the latest remote into our git cache" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1079:3-64 Comment False ("-- Use a local git clone to copy this git repo into a temp-dir" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1080:3-40 Comment False ("-- Delete the codebase in our temp-dir" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1081:3-93 Comment False ("-- Use sqlite's VACUUM INTO command to make a copy of the remote codebase into our temp-dir" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1082:3-76 Comment False ("-- Connect to the copied codebase and sync whatever it is we want to push." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1083:3-132 Comment False ("-- sync the branch to the staging codebase using `syncInternal`, which probably needs to be passed in instead of `syncToDirectory`" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1084:3-32 Comment False ("-- if setting the remote root," :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1085:3-50 Comment False ("--   do a `before` check on the staging codebase" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1086:3-40 Comment False ("--   if it passes, proceed (see below)" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1087:3-74 Comment False ("--   if it fails, throw an exception (which will rollback) and clean up." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1088:3-42 Comment False ("-- push from the temp-dir to the remote." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1089:3-25 Comment False ("-- Delete the temp-dir." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1090:3-4 Comment False ("--" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1091:3-25 Comment False ("-- set up the cache dir" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1094:5-73 Comment False ("-- Connect to codebase in the cached git repo so we can copy it over." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1096:7-77 Comment False ("-- Copy our cached remote database cleanly into our isolated directory." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1098:7-88 Comment False ("-- Connect to the newly copied database which we know has been properly closed and" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1099:7-36 Comment False ("-- nobody else could be using." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1113:7-69 Comment False ("-- the call to runDB \"handles\" the possible DB error by bombing" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1141:5-71 Comment False ("-- This function makes sure that the result of git status is valid." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1142:5-30 Comment False ("-- Valid lines are any of:" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1143:5-6 Comment False ("--" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1144:5-71 Comment False ("--   ?? .unison/v2/unison.sqlite3 (initial commit to an empty repo)" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1145:5-65 Comment False ("--   M .unison/v2/unison.sqlite3  (updating an existing repo)" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1146:5-81 Comment False ("--   D .unison/v2/unison.sqlite3-wal (cleaning up the WAL from before bugfix)" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1147:5-48 Comment False ("--   D .unison/v2/unison.sqlite3-shm (ditto)" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1148:5-6 Comment False ("--" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1149:5-30 Comment False ("-- Invalid lines are like:" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1150:5-6 Comment False ("--" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1151:5-41 Comment False ("--   ?? .unison/v2/unison.sqlite3-wal" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1152:5-6 Comment False ("--" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1153:5-64 Comment False ("-- Which will only happen if the write-ahead log hasn't been" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1154:5-49 Comment False ("-- fully folded into the unison.sqlite3 file." :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1155:5-6 Comment False ("--" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1156:5-62 Comment False ("-- Returns `Just (hasDeleteWal, hasDeleteShm)` on success," :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1157:5-64 Comment False ("-- `Nothing` otherwise. hasDeleteWal means there's the line:" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1158:5-40 Comment False ("--   D .unison/v2/unison.sqlite3-wal" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1159:5-54 Comment False ("-- and hasDeleteShm is `True` if there's the line:" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1160:5-40 Comment False ("--   D .unison/v2/unison.sqlite3-shm" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1161:5-6 Comment False ("--" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1180:5-25 Comment False ("-- Commit our changes" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1181:72-94 Comment True ("-- withIOError needs IO" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1183:7-30 Comment False ("-- has anything changed?" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1184:7-84 Comment False ("-- note: -uall recursively shows status for all files in untracked directories" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1185:7-38 Comment False ("--   we want this so that we see" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1186:7-52 Comment False ("--     `??  .unison/v2/unison.sqlite3` and not" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1187:7-27 Comment False ("--     `??  .unison/`" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1201:13-72 Comment False ("-- Only stage files we're expecting; don't `git add --all .`" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1202:13-59 Comment False ("-- which could accidentally commit some garbage" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1209:13-43 Comment False ("-- Push our changes to the repo" :| [])
parser-typechecker/src/Unison/Codebase/SqliteCodebase.hs:1216:3-62 Comment False ("-- remove any existing codebase at the destination location." :| [])

Environment

amesgen commented 2 years ago

Thanks for the report! If you just want to get rid of this failure, you can move the comment -- Remove this when the file is ready to be auto-formatted to the next line (such that {- ORMOLU_DISABLE -} does not have succeeding non-whitespace characters on its line), then everything works fine.

This is a minimized example for the issue:

{- ORMOLU_DISABLE -} -- foo

which is formatted

{- ORMOLU_DISABLE -}
-- foo

Here, we do not consider the input to contain a magic comment due to non-space characters being on the remaining line, see the guard (isAllSpace s3) line at the bottom:

https://github.com/tweag/ormolu/blob/b9eaa3d9a709b7467c444173ab9295f10608b614/src/Ormolu/Processing/Preprocess.hs#L166-L179

The AST does differ as the output then does contain a magic comment, so the input is a ParsedSnippet while the output is a RawSnippet.

The easiest fix I can think of is to consider such magic-ish comments to actually be magical, and to preserve the remaining line content; this issue report is some evidence that people expect this behavior. I implemented this fix in the amesgen/issue-856 branch.

Side note: Despite being my first suspicion, this issue is not related to the rework of disabled regions handling (#773), but does also occur on <0.3.0.0 for the same reasons.