Skip to content

Need help! Removing codeExprDoc from drasil-printers?#4509

Draft
balacij wants to merge 2 commits intomainfrom
rmCodeExprPrinters
Draft

Need help! Removing codeExprDoc from drasil-printers?#4509
balacij wants to merge 2 commits intomainfrom
rmCodeExprPrinters

Conversation

@balacij
Copy link
Collaborator

@balacij balacij commented Dec 6, 2025

I will annotate this PR for discussion, but this PR and branch are expected to be deleted in the end.

The goal of my work is to move the Drasil.Code.* modules from drasil-lang into drasil-code. However, that work is currently impossible because it would create a cyclic dependency between drasil-printers and drasil-code:

  • drasil-printers relies on CodeExpr and the other modules in Drasil.Code (namely code variables) for describing how CodeExpr can be rendered into Docs for human-readable, non-code purposes (I suppose ASCII variants).
  • drasil-code relies on drasil-printers for said rendering function and more (e.g., rendering functions for ModelExpr, Sentence, Symbol, etc.).

Comment on lines -33 to -35
-- | Create code expressions for a document in 'Doc' format.
codeExprDoc :: PrintingInformation -> SingleLine -> C.CodeExpr -> Doc
codeExprDoc pinfo sl e = pExprDoc sl (codeExpr e pinfo)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is used by drasil-code for a single purpose: rendering CodeExpr-based quantity constraints into human-readable text for outputting input value constraint violation messages (see stable/ changes).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like reasonable functionality - where does it belong? And maybe it should be in two stages (i.e. codeExprDoc would get fed the results of codeExpr e pinfo so that codeExpr would not be needed here).

{-# LANGUAGE GADTs #-}

-- | Defines functions to render 'CodeExpr's as printable 'P.Expr's.
module Language.Drasil.Printing.Import.CodeExpr (codeExpr) where
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I tried to delete this file at all is because, well, it is odd. CodeExpr is intended for rendering into GOOL (solely, to my knowledge), not necessarily rendering comments. Considering it is only used for a single purpose (the input constraint violation error messages), I'm not sure if the approach taken to outputting messages is a good one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong here as well!

But I neglected to also mention the initial reason I was looking at this code at all: drasil-printers and drasil-code have a cyclic dependency that we are currently avoiding through pushing some code from drasil-code into drasil-lang. This includes the "code variable chunks" and CodeExpr. My goal is to be able to move the drasil-code-related code from drasil-lang into drasil-code.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I certainly agree with that goal.

std::cout << ", but is expected to be ";
std::cout << "between ";
std::cout << 0.1;
std::cout << " (d_min)";
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While here we output a variable name in "implementation" Stage, in other areas, we have the flexibility of printing any general CodeExpr.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. But removing this output points to a real problem, i.e. these changes should not do that!

print(Float32(0.0))
print(" and ")
print(pi / Float32(2.0))
print(" ((pi)/(2))")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, $\frac{\pi}{2}$.

Comment on lines 419 to +430
printConstraint' _ (Range _ (Bounded (_, e1) (_, e2))) = do
lb <- convExpr e1
ub <- convExpr e2
return $ [printStr "between ", print lb] ++ printExpr e1 db ++
[printStr " and ", print ub] ++ printExpr e2 db ++ [printStrLn "."]
return $ [printStr "between "] ++ printExpr lb db ++
[printStr " and "] ++ printExpr ub db ++ [printStrLn "."]
printConstraint' _ (Range _ (UpTo (_, e))) = do
ub <- convExpr e
return $ [printStr "below ", print ub] ++ printExpr e db ++
return $ [printStr "below "] ++ printExpr ub db ++
[printStrLn "."]
printConstraint' _ (Range _ (UpFrom (_, e))) = do
lb <- convExpr e
return $ [printStr "above ", print lb] ++ printExpr e db ++ [printStrLn "."]
return $ [printStr "above "] ++ printExpr lb db ++ [printStrLn "."]
Copy link
Collaborator Author

@balacij balacij Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question to ask ourselves first is: What do we want the format of the constraint violation messages to be?

I somewhat like these, but I would also like to see CON$X$ (where CON$X$ is a label we defined in the SRS for a constraint) added as a prefix.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that more traceability in constraint violation messages would be even better.

@balacij
Copy link
Collaborator Author

balacij commented Dec 6, 2025

One solution to all of this is to split up drasil-printers. The rendering code for each language would move somewhere closer to the definition of said data types. We might still need a dependency-less drasil-printers-like library that the code can use for general "printing utilities," however.

Another solution (the one I decided on for this PR) is to delete codeExprDoc entirely. This way, we would need to figure out what we really want in our constraint violation messages.

However, if we wanted to maintain the status quo with this PR for the sake of maintaining the status quo, we should be able to avoid converting the constraint maps of Exprs into maps of CodeExprs until necessary for executable code generation. Leaving them as Exprs, we would be able to use our ASCII Expr printer to print out the same expressions (with one new configuration option: using the "implementation" stage versions of our variable symbols).

@balacij balacij changed the title Need help! Removing codeExpr from drasil-printers? Need help! Removing codeExprDoc from drasil-printers? Dec 6, 2025
convTypeOO, VisibilityTag(..))

import qualified Drasil.GOOL as OO (SFile)
import qualified Drasil.GOOL as OO (SFile, SValue)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These imports (of SFile and SValue), as well as ProcProg` all seem to point to something being odd here.

Comment on lines -33 to -35
-- | Create code expressions for a document in 'Doc' format.
codeExprDoc :: PrintingInformation -> SingleLine -> C.CodeExpr -> Doc
codeExprDoc pinfo sl e = pExprDoc sl (codeExpr e pinfo)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like reasonable functionality - where does it belong? And maybe it should be in two stages (i.e. codeExprDoc would get fed the results of codeExpr e pinfo so that codeExpr would not be needed here).

{-# LANGUAGE GADTs #-}

-- | Defines functions to render 'CodeExpr's as printable 'P.Expr's.
module Language.Drasil.Printing.Import.CodeExpr (codeExpr) where
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I certainly agree with that goal.

std::cout << ", but is expected to be ";
std::cout << "between ";
std::cout << 0.1;
std::cout << " (d_min)";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. But removing this output points to a real problem, i.e. these changes should not do that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants