Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix field accessors not showing up in view in some cases #4752

Merged
merged 3 commits into from Mar 8, 2024

Conversation

runarorama
Copy link
Contributor

For some types with lots of field accessors, they show up without field accessors when printed with edit or view, like this:

  type Mail
    = Mail
        [Personalization]
        MailAddress
        (Optional MailAddress)
        [MailAddress]
        Text
        [MailContent]
        (Optional [MailAttachment])
        (Optional Text)
        (Optional Json)
        (Optional (Map Text Text))
        (Optional [Text])
        (Optional Json)
        (Optional Instant)
        (Optional Text)
        (Optional Asm)
        (Optional Text)
        (Optional MailSettings)
        (Optional TrackingSettings)

This is caused by hashFieldAccessors returning Nothing for the first .set accessor. This is in turn caused by a type error in Typechecker.synthesize for that accessor. The note from the typechecker is:

The 2nd argument to `Mail.Mail`

          has type:  lib.base.Optional lib.base.time.Instant
    but I expected:  MailAddress

The term that's being typechecked looks like this:

(λ (User "_18". (λ (User "mail". (case Var User "mail" of [MatchCase {matchPattern = Constructor ReferenceDerived (Id "9mq7l0pr6foeuis6jujpo7hqcbsm5120lhu2nf70tccdtpuosmdfhr81foq9okme97b8kgd2h7k14pceu0nv9r2qjll416fiu5t06cg" 0) 0 [Unbound,Var,Var,Var,Var,Var,Var,Var,Var,Var,Var,Var,Var,Var,Var,Var,Var,Var], matchGuard = Nothing, matchBody = (User "_"-1. (User "_"-2. (User "_"-3. (User "_"-4. (User "_"-5. (User "_"-6. (User "_"-7. (User "_"-8. (User "_"-9. (User "_"-10. (User "_"-11. (User "_"-12. (User "_"-13. (User "_"-14. (User "_"-15. (User "_"-16. (User "_"-17. ConReferenceDerived (Id "9mq7l0pr6foeuis6jujpo7hqcbsm5120lhu2nf70tccdtpuosmdfhr81foq9okme97b8kgd2h7k14pceu0nv9r2qjll416fiu5t06cg" 0)#0 (Var User "_18") (Var User "_"-1) (Var User "_"-2) (Var User "_"-3) (Var User "_"-4) (Var User "_"-5) (Var User "_"-6) (Var User "_"-7) (Var User "_"-8) (Var User "_"-9) (Var User "_"-10) (Var User "_"-11) (Var User "_"-12) (Var User "_"-13) (Var User "_"-14) (Var User "_"-15) (Var User "_"-16) (Var User "_"-17))))))))))))))))))}])))))

This fix adds an ordinal to the variable names of the fields, so freshen should in most cases be a no-op.

After this fix the type above is rendered as:

  type Mail
    = { personalizations : [Personalization],
        from : MailAddress,
        replyTo : Optional MailAddress,
        replyToList : [MailAddress],
        subject : Text,
        content : [MailContent],
        attachments : Optional [MailAttachment],
        templateId : Optional Text,
        sections : Optional Json,
        headers : Optional (Map Text Text),
        categories : Optional [Text],
        customArgs : Optional Json,
        sendAt : Optional Instant,
        batchId : Optional Text,
        asm : Optional Asm,
        ipPoolName : Optional Text,
        mailSettings : Optional MailSettings,
        trackingSettings : Optional TrackingSettings }

as they're not freshened properly
@ChrisPenner
Copy link
Contributor

Sorry can you explain in simpler terms what the problem was here? Were there conflicting type var names that caused it to fail to type-check, and if that's the case, is freshIn broken or was it just not scoped properly?

Great find either way BTW 🙌🏼

@aryairani
Copy link
Contributor

aryairani commented Mar 8, 2024

Sorry can you explain in simpler terms what the problem was here? Were there conflicting type var names that caused it to fail to type-check, and if that's the case, is freshIn broken or was it just not scoped properly?

Great find either way BTW 🙌🏼

@ChrisPenner Our guess is that, for either misguided or arbitrary reasons, the term printer was not printing same-named-but-differently-fresh variables with distinct names, apart from pattern vars. This PR sidesteps that issue by giving these variables differently-named vars, instead of same-named-but-differently-fresh vars.

@aryairani aryairani merged commit d913a61 into trunk Mar 8, 2024
7 checks passed
@aryairani aryairani deleted the runarorama/fixFieldNames branch March 8, 2024 15:09
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.

None yet

3 participants