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

change hir::ExprLit to take a ConstVal instead of an ast::Lit #32793

Closed
wants to merge 5 commits into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Apr 7, 2016

This PR uses the lowering step to move from ast::Lit to ConstVal. Since suffixed literals (e.g. 42_u8) are turned into concrete ConstInt variants, these literal overflow checks need to be done in the ast. This causes a little code duplication in the rustc_lint/types lints.

I don't know how immutable the HIR is, but if we can run a mutating pass after typeck, we could also lower all ConstInt::Infer/ConstInt::InferSigned variants to their concrete variants.

r? @eddyb

As a next step I can remove some code-duplication between MIR-trans and normal trans (since MIR already uses ConstVal instead of literals).

}
}

pub fn lower_un_neg(lctx: &LoweringContext, inner: &Expr) -> (hir::Expr_, ThinAttributes) {
Copy link
Member

Choose a reason for hiding this comment

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

Is all of this necessary? Can't we just keep the negation expression?

Copy link
Contributor

Choose a reason for hiding this comment

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

This works better in patterns

@arielb1
Copy link
Contributor

arielb1 commented Apr 7, 2016

I am quite sure the HIR is supposed to be immutable

@eddyb
Copy link
Member

eddyb commented Apr 7, 2016

I think everything which wants accurate information should use MIR as the source of truth - maybe the lints could be triggered from the MIR building code?

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 7, 2016

@eddyb triggering lints manually is weird

I have a different suggestion:

  • during AST -> HIR lowering do the following conversions
AST HIR breaking change?
LitKind::Int(n, Unsuffixed) ConstInt::Infer(n) -
ExprKind::UnOp(Neg, LitKind::Int(n, Unsuffixed)) ConstInt::InferSigned(-n) Yes, for negated literals bigger than i64::MAX. Note, we already error for all literals bigger than u64::MAX
LitKind::Int(n, t) ExprType(ConstInt::Infer(n), t) -
ExprKind::UnOp(Neg, LitKind::Int(n, t)) ExprType(ConstInt::InferSigned(-n), t)) Yes, see above
  • during HIR -> MIR lowering do nothing
  • add a MIR-pass that folds numeric constants and lints on bad things.

@eddyb
Copy link
Member

eddyb commented Apr 7, 2016

@oli-obk You can't really lint after you've built the MIR in the same way because you lose location information (spans are there, but no IDs to get lint attributes for - maybe we need to track lints some other way? in spans? cc @nrc).

@@ -1145,10 +1152,17 @@ fn lit_to_const<'tcx>(lit: &ast::LitKind,
infer(Infer(n), tcx, &ty::TyUint(ity), span).map(Integral)
},

LitKind::Float(ref n, _) |
LitKind::Float(ref n, fty) => {
if let Ok(x) = n.parse::<f64>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to do a double-rounding?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why anyone would want to do that, but ConstVal doesn't have separate variants for f64 and f32 so avoiding the double rounding doesn't seem possible without semi-large changes all throughout const eval. I've been meaning to find a short example program that demonstrates the double-rounding and file an issue. In any case, fixing this existing brokenness seems orthogonal to and out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's #32805 which is vaguely relevant.

@bors
Copy link
Contributor

bors commented Apr 7, 2016

☔ The latest upstream changes (presumably #32016) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 13, 2016

@eddyb I could do these changes without touching MIR. I'll simply reinstate the current HIR-lint.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 4, 2016

closing until things have settled around lints and MIR

@oli-obk oli-obk closed this May 4, 2016
@oli-obk oli-obk deleted the const_hir_lit branch June 15, 2020 15:25
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.

5 participants