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

typeck should safe-guard against types with un-substituted formals #13139

Closed
pnkfelix opened this issue Mar 25, 2014 · 6 comments
Closed

typeck should safe-guard against types with un-substituted formals #13139

pnkfelix opened this issue Mar 25, 2014 · 6 comments
Labels
A-type-system Area: Type system P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

Spawned off of #5121.

Part of the bug underlying #5121 lay dormant for quite a while because some of the type-checking machinery is not as aggressive as it could be about ensuring that the types it is checking actually correspond to concrete types.

In particular, in a concrete function type, the bound-type/lifetimes (i.e. the "formals" in P.L. speak) from the function's signature have been replaced with other concrete types (i.e. the "actuals").

Currently, the ty represetnation does not really distinguish between formals and actuals except for lifetimes. But nonetheless, we can at least safe-guard against this problem in that case (and presumably if we later change ty to between distinguish between formal/actual for type variables as well, then similar checks would go into the same place, probably).

@nikomatsakis
Copy link
Contributor

The way I had planned to solve this is to use distinct types for "types that may contain inference values" and "types that do not". I actually had a branch of rustc a couple of years back building that did precisely this. The basic idea is to parameter ty::sty with a type parameter that is the type for "reference to another type". Then we pull out all the inference types into a distinct enum that wraps the base enum:

enum sty<T> { ty_tup(~[T]), ... } 
struct normal_type { ... sty<normal_type> ... }
struct inference_type { ... sty<inference_type> ... }

This way, the types of an ast item are distinct from those used during inference. You can write folders and so on generically over T (in fact, lacking default methods, this was INCREDIBLY PAINFUL which is part of the reason I never tried to land the branch, but it should be easier now).

You can see some of the groundwork from this branch still:

  • function contexts have their own distinct tables for types
  • the core folder is written as a trait with default methods

But one of the challenges I remember was having to find the right canonicalization scheme. I just didn't canonicalize during inference but instead allocated fresh @ty everywhere. This was slow. I suspect using an arena per function would be the right answer here.

@reem
Copy link
Contributor

reem commented Dec 1, 2014

Triage: I'm not familiar with the relevant internals here and #5121 is closed, so I'm not sure what is still left to do here.

@nikomatsakis
Copy link
Contributor

Nothing has really changed. Still a sort of "nice to have" refactoring though I think, most likely, we'd want to just roll this into whatever "next generation" design we come up with for type checking.

@steveklabnik
Copy link
Member

Triage: no idea if this has changed in the last year.

@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 7, 2015
@brson brson added P-low Low priority I-wishlist labels Apr 11, 2017
@brson
Copy link
Contributor

brson commented Apr 11, 2017

@pnkfelix @nikomatsakis is this valuable to have open?

@nikomatsakis
Copy link
Contributor

I think we can close this.

matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this issue Sep 13, 2022
Suggest struct when completing enum

closes rust-lang#13107
flip1995 pushed a commit to flip1995/rust that referenced this issue Jul 25, 2024
…ut, r=Alexendoo

Lintcheck: Rework and limit diff output for GH's CI

### Background

While working on rust-lang/rust-clippy#13136 I found an amazing limitation of GH's CI. The summary can at most have be 1MB of text. Here is the warning message:

> $GITHUB_STEP_SUMMARY upload aborted, supports content up to a size of 1024k, got 46731k. For more information see: https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary

[The PR](rust-lang/rust-clippy#13136) produced a *casual* 61808 changes. Guess that's why those lints are not *warn-by-default* :P.

### Changes:

This PR limits the lintcheck diff output in two ways.

1. The diff is limited to 200 messages per lint per section. Hidden messages are indicated by a message at the end of the section.
2. The output is first written to a file and only the first 1MB is written to ` >> $GITHUB_STEP_SUMMARY`. The entire file is also written to the normal CI log. This helps for cases where several lints change and the total size exceeds the 1MB limit.

An example of these changes can be seen here: https://github.com/xFrednet/rust-clippy/actions/runs/10028799118?pr=4

---

changelog: none

r? `@Alexendoo`

Sorry for bombarding you with so many PR's lately 😅 Feel free to pass some of you reviews to me.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-type-system Area: Type system P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants