misc build cutoff fixes and add --no-cutoff flag · purescript/purescript@aa9e76d · GitHub
Skip to content

Commit aa9e76d

Browse files
thomashoneymanthomashoneyman
committed
misc build cutoff fixes and add --no-cutoff flag
fixes issues found in the incremental builds, via comparing incremental vs. clean builds on example projects like prelude Each of these caused a stale but successful build: - Re-exports via unqualified imports (`module B (module A) where import A`, the common prelude pattern) were not matched against downstream usage, so changes or removals flowing through such re-exports failed to rebuild consumers. - Newly added re-exports were absent from the externs diff, so a name newly brought into scope could not trigger a rebuild of a module it made ambiguous. - do/ado notation and unary minus desugar into references to bind, discard, map, apply, pure and negate that are not present in the parsed module, so changes to those names went undetected. Rename handling: - Re-stamp codegen outputs after patching externs in place, otherwise every patched module was needlessly recompiled on the next build. - Patch instance chain ids as well as source spans, otherwise a moved file's old path lingered in externs and incremental output diverged from a clean build. - Rebuild a moved module when corefn, docs or source-map targets are requested, since those embed the module's own source path. This also adds a --no-cutoff flag to `purs compile` to rebuild everything downstream of a change, matching older compilers, to help diagnose an unexpected build result. It also restores the cut-off test cases dropped in an earlier refactor, ports the regression tests from the purserl fork, and adds some more adversarial and rename and --no-cutoff tests in TestMake.hs
1 parent 1893622 commit aa9e76d

7 files changed

Lines changed: 618 additions & 21 deletions

File tree

‎app/Command/Compile.hs‎

Lines changed: 12 additions & 0 deletions

‎src/Language/PureScript/Make.hs‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ import Language.PureScript.Make.ExternsDiff qualified as ED
5151
import Language.PureScript.Make.Monad as Monad
5252
import Language.PureScript.ModuleDependencies (DependencyDepth (..), moduleSignature, sortModules')
5353
import Language.PureScript.Names (ModuleName (..), isBuiltinModuleName, runModuleName)
54+
import Language.PureScript.Options (CodegenTarget (..))
5455
import Language.PureScript.Renamer (renameInModule)
5556
import Language.PureScript.Sugar (Env, collapseBindingGroups, createBindingGroups, desugar, desugarCaseGuards, externsEnv, primEnv)
5657
import Language.PureScript.TypeChecker (CheckState (..), emptyCheckState, typeCheckModule)
@@ -320,7 +321,21 @@ make' MakeOptions{..} ma@MakeActions{..} ms = do
320321

321322
catchFailure moduleIndex $ do
322323
-- Get the reason for building or skipping the compilation.
323-
case BuildPlan.getBuildReason buildPlan m depsDiffs of
324+
noCutoff <- askNoCutoff
325+
buildAction <- case BuildPlan.getBuildReason noCutoff buildPlan m depsDiffs of
326+
-- Patching externs in place is only sufficient for a moved
327+
-- module when JS is the only codegen target: corefn, docs and
328+
-- source maps embed the module's own source path and would go
329+
-- stale, so in that case rebuild instead. Downstream modules
330+
-- carry a moved dependency's path only in their externs, which
331+
-- are patched, so they can be patched either way.
332+
action@(PatchExterns _ _ (Just _)) -> do
333+
targets <- askCodegenTargets
334+
pure $ if S.isSubsetOf targets (S.singleton JS)
335+
then action
336+
else RebuildBecause SourceFileMoved
337+
action -> pure action
338+
case buildAction of
324339
-- No rebuild reason skipping the module.
325340
SkipBuild exts warnings -> do
326341
_ <- updateOutputTimestamp moduleName Nothing
@@ -330,6 +345,11 @@ make' MakeOptions{..} ma@MakeActions{..} ms = do
330345

331346
PatchExterns patchedExterns patchedWarnings fileNameDiff -> do
332347
replaceExterns patchedExterns
348+
-- Writing the patched externs bumps its modification time, so
349+
-- the other output files have to be re-stamped along with it:
350+
-- otherwise the next build considers the codegen outputs
351+
-- outdated and needlessly recompiles the module.
352+
_ <- updateOutputTimestamp moduleName Nothing
333353
progress $ SkippingModule moduleName moduleIndex
334354
pure $ BuildJobSucceeded Nothing patchedWarnings patchedExterns (Just (ED.moduleMovedDiff moduleName fileNameDiff))
335355

‎src/Language/PureScript/Make/Actions.hs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,13 @@ data RebuildReason
9292
| LaterDependency ModuleName
9393
-- | Compiled because of (the first found) changed reference in a dependency.
9494
| UpstreamRef ED.DiffRef
95+
-- | Compiled because the module's source file moved and requested codegen
96+
-- targets (corefn, docs, source maps) embed the source path, so patching
97+
-- the externs alone would leave them stale.
98+
| SourceFileMoved
99+
-- | Compiled because incremental cut-off was disabled (--no-cutoff) and this
100+
-- module is downstream of a rebuilt module.
101+
| CutOffDisabled
95102
deriving (Show, Eq, Ord)
96103

97104
-- | Progress messages from the make process
@@ -222,6 +229,10 @@ data MakeActions m = MakeActions
222229
-- load .js files as ES modules.
223230
, outputPrimDocs :: m ()
224231
-- ^ If generating docs, output the documentation for the Prim modules
232+
, askCodegenTargets :: m (S.Set CodegenTarget)
233+
-- ^ Get the currently requested codegen targets.
234+
, askNoCutoff :: m Bool
235+
-- ^ Whether incremental build cut-off is disabled.
225236
}
226237

227238
-- | Given the output directory, determines the location for the
@@ -333,6 +344,8 @@ buildMakeActions outputDir filePathMap foreigns usePrefix =
333344
writeCacheDb
334345
writePackageJson
335346
outputPrimDocs
347+
(asks optionsCodegenTargets)
348+
(asks optionsNoCutoff)
336349
where
337350

338351
getInputTimestampsAndHashes

‎src/Language/PureScript/Make/BuildPlan.hs‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ import Data.Maybe (catMaybes, fromMaybe, isNothing)
2727
import Data.Set qualified as S
2828
import Data.Text qualified as T
2929
import Data.Time.Clock (UTCTime)
30-
import Language.PureScript.AST (Module, getModuleName, getModuleSourceSpan, SourceSpan, spanName)
30+
import Language.PureScript.AST (Module, getModuleName, getModuleSourceSpan, modifySpanName, spanName)
31+
import Language.PureScript.AST.Declarations.ChainId (ChainId (..))
3132
import Language.PureScript.CST qualified as CST
3233
import Language.PureScript.Crash (internalError)
3334
import Language.PureScript.Errors (MultipleErrors (..))
@@ -173,11 +174,14 @@ data BuildAction
173174
| RebuildBecause RebuildReason
174175

175176
-- Get the build reason.
176-
getBuildReason :: BuildPlan -> Module -> Maybe [ExternsDiff] -> BuildAction
177-
getBuildReason _ _ Nothing = RebuildBecause NoCachedDependency
178-
getBuildReason (BuildPlan {..}) m (Just depsDiffs) =
177+
getBuildReason :: Bool -> BuildPlan -> Module -> Maybe [ExternsDiff] -> BuildAction
178+
getBuildReason _ _ _ Nothing = RebuildBecause NoCachedDependency
179+
getBuildReason noCutoff (BuildPlan {..}) m (Just depsDiffs) =
179180
case M.lookup mn bpPrevious of
180181
Just (Just reason, _) -> RebuildBecause reason
182+
-- With cut-off disabled, any module that is a candidate for skipping (up to
183+
-- date but downstream of a rebuilt module) is rebuilt unconditionally.
184+
Just (Nothing, _) | noCutoff -> RebuildBecause CutOffDisabled
181185
Just (Nothing, exts) ->
182186
let
183187
externs = pbExterns exts
@@ -195,8 +199,13 @@ getBuildReason (BuildPlan {..}) m (Just depsDiffs) =
195199
fileNameFromExterns = spanName . efSourceSpan
196200
fileNameFromModule = spanName . getModuleSourceSpan
197201

198-
patchExterns = over template :: (SourceSpan -> SourceSpan) -> ExternsFile -> ExternsFile
199-
patchWarnings = over template :: (SourceSpan -> SourceSpan) -> MultipleErrors -> MultipleErrors
202+
-- Source file names appear in externs both in source spans and inside
203+
-- instance chain ids, so both have to be patched.
204+
patchSpans = over template . modifySpanName :: (String -> String) -> ExternsFile -> ExternsFile
205+
patchChainIds = over template . patchChainId :: (String -> String) -> ExternsFile -> ExternsFile
206+
patchChainId f (ChainId (fp, pos)) = ChainId (f fp, pos)
207+
patchExterns f = patchChainIds f . patchSpans f
208+
patchWarnings f = over template (modifySpanName f) :: MultipleErrors -> MultipleErrors
200209

201210
data Options = Options
202211
{ optPreloadAllExterns :: Bool

‎src/Language/PureScript/Make/ExternsDiff.hs‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,14 +45,14 @@ import Data.String (String)
4545

4646
import Language.PureScript.AST qualified as P
4747
import Language.PureScript.AST.Declarations.ChainId (ChainId (..))
48+
import Language.PureScript.Constants.Libs qualified as C
4849
import Language.PureScript.Constants.Prim (primModules)
4950
import Language.PureScript.Crash (internalError)
5051
import Language.PureScript.Environment qualified as P
5152
import Language.PureScript.Externs qualified as P
5253
import Language.PureScript.Names (ModuleName)
5354
import Language.PureScript.Names qualified as P
5455
import Language.PureScript.Types qualified as P
55-
import Language.PureScript.AST.SourcePos (modifySpanName)
5656

5757
-- Refs structure appropriate for storing and checking externs diffs.
5858
data Ref
@@ -96,10 +96,12 @@ isRefRemoved Removed = True
9696
isRefRemoved _ = False
9797

9898
-- To get changed reexported refs, we take those which were removed (not
99-
-- present in new extern's exports) or changed in dependencies.
99+
-- present in new extern's exports) or changed in dependencies. Refs that are
100+
-- newly reexported are also included (as Added): they may conflict with
101+
-- names already in scope in downstream modules.
100102
getReExported :: P.ExternsFile -> P.ExternsFile -> ModuleRefsMap -> RefsWithStatus
101103
getReExported newExts oldExts depsDiffsMap =
102-
M.fromList $ mapMaybe checkRe $ S.toList oldExports
104+
M.union (M.fromList $ mapMaybe checkRe $ S.toList oldExports) addedRefs
103105
where
104106
goRe (P.ReExportRef _ es ref) = S.mapMonotonic (P.exportSourceDefinedIn es,) (toRefs ref)
105107
goRe _ = S.empty
@@ -110,6 +112,8 @@ getReExported newExts oldExts depsDiffsMap =
110112
| (mn, ref) `notElem` newReExports = Just (ref, Removed)
111113
| Just True <- elem ref <$> M.lookup mn depsDiffsMap = Just (ref, Updated)
112114
| otherwise = Nothing
115+
addedRefs =
116+
M.fromSet (const Added) $ S.map snd (S.difference newReExports oldExports)
113117

114118
-- Extracts declarations from old and new externs and compares them. Returns a
115119
-- tuple of changed refs (a form of which have changed) and unchanged refs with
@@ -273,15 +277,17 @@ data DiffRef
273277

274278
data DiffAction
275279
= NoChange
276-
| Patch (P.SourceSpan -> P.SourceSpan)
280+
| -- | Maps old source file names to new ones; the previous build result
281+
-- can be reused with the file names patched.
282+
Patch (String -> String)
277283
| DiffRef DiffRef
278284

279285
-- Checks if the module effectively uses any of diff's refs.
280286
checkDiffs :: P.Module -> Maybe (String, String) -> [ExternsDiff] -> DiffAction
281287
checkDiffs (P.Module _ _ _ decls exports) moduleNameDiff diffs
282288
| Just renames <- M.fromList . catMaybes . (moduleNameDiff :) <$> traverse isTrivial diffs
283289
= if M.null renames then NoChange else Patch $
284-
modifySpanName $ \name -> fromMaybe name (M.lookup name renames)
290+
\name -> fromMaybe name (M.lookup name renames)
285291
| otherwise = case makeSearches decls diffs of
286292
Left r -> DiffRef (ImportedRef r)
287293
Right searches
@@ -292,14 +298,18 @@ checkDiffs (P.Module _ _ _ decls exports) moduleNameDiff diffs
292298
where
293299
noRealChange = case moduleNameDiff of
294300
Nothing -> NoChange
295-
Just (prev, next) -> Patch $ modifySpanName $ \name -> if name == prev then next else name
296-
-- Check if the module reexports any of searched refs.
301+
Just (prev, next) -> Patch $ \name -> if name == prev then next else name
302+
-- Check if the module reexports any of searched refs. A `module M`
303+
-- export reexports refs imported under the qualifier M, as well as refs
304+
-- imported from the module M itself without qualification (the common
305+
-- `module B (module A) where import A` prelude pattern).
297306
checkReExports searches =
298307
map (\(mn, _, ref) -> (mn, ref)) $
299308
exports >>=
300309
listToMaybe . foldMap
301310
( \case
302-
P.ModuleRef _ mn -> maybeToList $ find (\(_, mn' ,_) -> mn' == Just mn ) searches
311+
P.ModuleRef _ mn -> maybeToList $
312+
find (\(mnDef, mn', _) -> mn' == Just mn || (isNothing mn' && mnDef == mn)) searches
303313
_ -> []
304314
)
305315

@@ -356,8 +366,19 @@ checkUsage searches decls = listToMaybe anyUsages
356366
| otherwise -> checkValue n
357367
P.Constructor _ n -> checkCtor n
358368
P.Op _ n -> checkValueOp n
369+
-- Do/ado notation and unary minus are desugared later into references
370+
-- to `bind`/`discard`, `map`/`apply`/`pure` and `negate` respectively
371+
-- (qualified for qualified do/ado), so those hidden usages have to be
372+
-- searched here explicitly.
373+
P.Do mbQual _ ->
374+
foldMap (checkValue . mkQualified mbQual) [C.S_bind, C.S_discard]
375+
P.Ado mbQual _ _ ->
376+
foldMap (checkValue . mkQualified mbQual) [C.S_map, C.S_apply, C.S_pure]
377+
P.UnaryMinus _ _ -> checkValue (mkQualified Nothing C.S_negate)
359378
_ -> mempty
360379

380+
mkQualified mbQual = P.Qualified (P.byMaybeModuleName mbQual) . P.Ident
381+
361382
goBinder _ binder = case binder of
362383
P.ConstructorBinder _ n _ -> checkCtor n
363384
P.OpBinder _ n -> checkValueOp n

‎src/Language/PureScript/Options.hs‎

Lines changed: 4 additions & 1 deletion

0 commit comments

Comments
 (0)